ltilve / chromium Goto Github PK

This problem is not totally deterministic, but in some cases occurred that adding to the overflow one extension from the toolbar context menu, unchecks the pressed browserAction.

If the contents of an extension sidebar have been inspected with the devtools, closing the sidebar is generating a segmentation fault.

As only one sidebar can be shown at once, it's enough for making explicit the extension responsible for it.

There are build issues after the last changes applied at 5b4e587 to port at the cocoa part the perWindow approach.

https://codereview.chromium.org/1168383002/#msg5

You should also include the logic from ExtensionActionViewController, since that
class is platform-agnostic, and does not require a real view - thus it's part of
the "core" logic. Then, we can have a slightly more intensive test, like
clicking on actions in order to open/close sidebars.

This case is only present when extensions are opened inside a sidebar, as right-click on the browserAction button closes the popup with the default behaviour.

By the comments from Devlin https://codereview.chromium.org/1168383002/#msg20

What about testing multiple extensions with sidebars?

Also review if we can manage to have the tests running inside of unittests instead of the browser ones

By https://codereview.chromium.org/1152613003/diff/20001/chrome/browser/chrome_notification_types.h#newcode343

We're working "hard" (read: we really want to, but bandwidth is hard to come by)
to deprecate notifications, so it'd be best to avoid adding any more, if we can.

It seems there are some problems on the behaviour of some extensions when their popup is shown as a sidebar. For instance 'Process sample' https://developer.chrome.com/extensions/samples#search:process%20monitor is working as expected on both cases, others as 'My Bookmarks' https://developer.chrome.com/extensions/samples#search:my%20bookmarks are apparently failing to populate the sidebar page. This issue seems to be happening also with "bookmarks-sidebar" example extension when the sidebar is not displayed by using the chrome.sidebar.show API call.

By https://codereview.chromium.org/1152613003/#msg15

https://codereview.chromium.org/1152613003/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/sidebar_manager.cc:191: void
SidebarManager::NavigateSidebar(content::WebContents* tab,
Certain methods here (like this one) seem to belong purely on the Sidebar
itself, rather than in the manager. I think the Manager should be responsible
for managing which sidebar is showing (if any), but not responsible for the
internal state of that sidebar (like url).

From https://codereview.chromium.org/1152613003/diff/40001/chrome/browser/extensions/sidebar_manager.cc?context=10&column_width=80&tab_spaces=8

I don't think we should support multiple sidebars in a single tab - it's
wasteful with web contents, and this "active/inactive" stuff complicates the
code a lot. Why not just have it behave similar to popups (though on a per-tab
basis) - you can have one sidebar open at a time, and if you open another, it
closes the first (completely)?

• out/Debug/interactive_ui_tests --no-sandbox --single_process --gtest_filter=SidebarTest.*

Crashes at SidebarTest::ShowSidebar(WebContents* temp, const std::string& test_page),
Due to the null host_ in SidebarContainer.
We need to check initialization sequence of SidebarContainer and SidebarManager

AddObserver should be done at construction time of ExtensionActionViewController,
to match with RemoveObserver in destructor of ExtensionActionViewController.
So we need to remove AddObserver from TriggerPopupWithUrl

It is handled at cd41a1e0c4f461d01b75a5ec425eab20787bbf10

Also, We need to handle popup_owner_ of ToolbarActionBar properly.

Implement proper codes instead of hard coded value with comment:

void BrowserView::UpdateSidebarForContents(content::WebContents* new_contents) {
...
int sidebar_width = 300;// g_browser_process->local_state()->GetInteger(
//prefs::kExtensionSidebarWidth);

After this CL https://codereview.chromium.org/1105713002, the popups that are created using the openPopup() api calls, are sliding out of the overflow while the popup is visible. When the extension is inside the overflow, the hamburger menu it's depressed (as for the browserAction button visible case), but it's not behaving in the same way as for standard popups.

../../third_party/tcmalloc/chromium/src/free_list.h:118] Memory corruption detected.
Segmentation fault

The crash happened once after an extension was being displayed as a sidebar on the newTab window, and called window.close() after a few seconds timeout. Repeating the process several times more, I was not able to reproduce again the problem.

When the new toolbar design is not enabled at chrome://flags/#enable-extension-action-redesign, closing the popup of a page-action extension (for instance this, that shows a clickable pageaction when the url contains a 'g' http://people.igalia.com/ltilve/pageaction_by_url.tgz), crashes the browser.

https://codereview.chromium.org/1168383002/#msg11

https://codereview.chromium.org/1168383002/diff/60001/chrome/browser/extensions/sidebar_manager.h#newcode95

Having two maps is probably excessive, since most users won't have that many
sidebars open, and keeping everything up to date is a bit of a pain. I'd just
have one map, keyed on web contents, and then if you need to look up a
particular sidebar, you can just traverse it (since it will be small).

Instead of allowing a browserAction flag to affect globally all the extensions, each one might determine if it's displayed inside of a sidebar 0ccf0eb 3a0650b

https://codereview.chromium.org/1168383002/diff/60001/chrome/browser/extensions/sidebar_manager.cc#newcode109

This should be accomplished without notifications. Would TabStripModelObserver
work here?

From Devlin review:
https://codereview.chromium.org/1152613003/diff/20001/chrome/browser/browser_process.h#newcode124

We almost certainly don't want to have a SidebarManager as part of the browser
process (sidebars cannot, for instance, cross profiles). This should be tied
more closely to a profile, and should probably either be a
BrowserContextKeyedService (or one of its subclasses) or live on the
ExtensionSystem. My first inclination is probably that the latter makes more
sense.

Currently when an extension wants to close itself by calling window.close() and is inside a sidebar, it's not being properly handled.

When we have an tab with a visible extension sidebar, and that tab is dragged from the chromium window, and dropped into another browser window (or to a new window) the browser action button keeps the pressed state. The crash is not happening any more after the fix for #27 but still happening this issue.

jquery adds 9205 lines in single CL which makes hard to review it.
Because the example is not so complex to use jquery, let's remove it.

Fixing addressed reviews at https://codereview.chromium.org/1168383002/#msg13

There is a segmentation fault when we have an tab with a visible extension sidebar, and that tab is dragged from the chromium window, and dropped into another browser window.

Using the chrome.tabs API to iterate through the available tabs to toggle the sidebar at all of them, it also shows it at the devTools window, when this is detached. It might be a good idea to avoid allowing sidebars at the DevTools, as there is already an specific chrome.devtools.panels API for that.

From https://codereview.chromium.org/1152613003/#msg15

I think we should split it up into (at least) 3. The first one should include
no "real" UI code - that is, it shouldn't have anything touched in ui/views or
ui/cocoa, and you can probably leave out a little of the stuff in, e.g.,
ui/browser. The second should add the views implementation, and the third
should add the cocoa implementation.

out/Debug/browser_tests --gtest_filter=BrowserActionsBarBrowserTest.*
out/Debug/browser_tests --gtest_filter=BrowserActionsBarRedesignBrowserTest.*

Shows failures on top of igalia/sidebar-cocoa a333115911cd2577004f6dc42c1af2caa8695a94

Those classes should not placed in extensions/common, because these functionality are
only related to sidebar_api and sidebar_manager

extensions/common/extension_sidebar_defaults.cc | 18 +
extensions/common/extension_sidebar_defaults.h | 29 +
extensions/common/extension_sidebar_utils.cc | 37 +
extensions/common/extension_sidebar_utils.h | 29 +

$ ./out/Release/unit_tests --single-process --gtest_filter=BrowserWindowControllerTest*

[ RUN ] BrowserWindowControllerTest.TestSplitViewsAreNotOpaque
2015-04-21 16:10:45.261 unit_tests[49694:1591922] -[SidebarController view]: unrecognized selector sent to instance 0x7ff88e855b30
[49694:1299:0421/161045:79539930151365:FATAL:chrome_browser_application_mac.mm(159)] Someone is trying to raise an exception! NSInvalidArgumentException reason -[SidebarController view]: unrecognized selector sent to instance 0x7ff88e855b30
[11/17] BrowserWindowControllerTest.TestResizeViewsWithBookmarkBar (171 ms)
[12/17] BrowserWindowControllerTest.BookmarkBarIsSameWidth (79 ms)
[13/17] BrowserWindowControllerTest.TestTopRightForBubble (70 ms)
[14/17] BrowserWindowControllerTest.TestZoomFrame (116 ms)
[15/17] BrowserWindowControllerTest.TestFindBarOnTop (84 ms)
[16/17] BrowserWindowControllerTest.TestSplitViewsAreNotOpaque (CRASHED)
Note: Google Test filter = BrowserWindowControllerTest.BookmarkBarHitTest
[==========] Running 1 test from 1 test case.
[----------] Global test environment set-up.
[----------] 1 test from BrowserWindowControllerTest
[ RUN ] BrowserWindowControllerTest.BookmarkBarHitTest
[ OK ] BrowserWindowControllerTest.BookmarkBarHitTest (150 ms)
[----------] 1 test from BrowserWindowControllerTest (151 ms total)

[----------] Global test environment tear-down
[==========] 1 test from 1 test case ran. (151 ms total)
[ PASSED ] 1 test.
[17/17] BrowserWindowControllerTest.BookmarkBarHitTest (150 ms)
1 test crashed:
BrowserWindowControllerTest.TestSplitViewsAreNotOpaque
Tests took 2 seconds.

https://codereview.chromium.org/1168383002/#msg7