Closed ltilve closed 9 years ago
ryumiel
commented
9 years ago It looks like chrome.bookmark.getTree does not work in the sidebar mode.
If I execute below statement in extension's inspector chrome.bookmarks.getTree(function(s){ alert(s); }); in popup mode it shows alert with [Object object], but in sidebar mode, this callback doesn't executed.
Maybe the permission handling for extension is not working properly in the sidebar mode.
ryumiel
commented
9 years ago Another interesting point is there are sidebar.js and popup.js in bookmark-sidebar extension, but it only uses popup.js in popup and sidebar mode. Maybe first intension of this extension is providing different UI and functionality between popup and sidebar.
We need to make a decision whether should we support this functionality or remove sidebar.js and sidebar.html in bookmark-sidebar extension.
ryumiel
commented
9 years ago In popup mode, chrome.bookmarks.getTree is reaching to BookmarksGetTreeFunction::RunOnReady() via below callstack. But in sidebar mode, it even does not reach to void ExtensionFunctionDispatcher::DispatchWithCallbackInternal. (it is #4 stack)
ryumiel
commented
9 years ago Possible suspect.
bool WebContentsImpl::OnMessageReceived(RenderViewHost* render_view_host, RenderFrameHost* render_frame_host, const IPC::Message& message) { DCHECK(render_view_host || render_frame_host); if (GetWebUI() && static_cast<WebUIImpl*>(GetWebUI())->OnMessageReceived(message)) { return true; }
ObserverListBase
Maybe in sidebar mode, we do not add WebContentsObserver
ltilve
commented
9 years ago There are problems running some extensions inside a sidebar, instead of with a popup, due to apparent certain calls to chrome API not working (eg., getting the bookmarks). It is not happening for all the cases, so still investigation in progress.
According to the backtrace. When calling the bookmarks API, this go through IPC, and it is intercepted by ExtensionHost, which unmarshall it and do the proper call.
This happens when having a popup. But when using a sidebar, basically we are not creating an ExtensionHost, so we don't handle that call.
Following the code from ExtensionActionViewController::TriggerPopupWithURL() an ExtensionViewHost is created through ExtensionViewHostFactory::CreatePopupHost() (line 346). Checking this factory, there is a similar call for creating a dialogue. Probably we should add here a new method, CreateSidebarHost().
Tracking ExtensionViewHostFactory::CreatePopupHost() it ends up creating a ExtensionViewHost, which inherits from ExtensionHost.
Taking a look at ExtensionHost constructor, surprisingly it is very similar to the code in SidebarContainer constructor.
SidebarContainer maybe should have been implemented as an ExtensionHost. We could consider moving the code from SidebarContainer to ExtensionHost, but I it is probably safer if we just call the proper methods in SidebarManager/SidebarContainer from ExtensionHost.
So summing up, when creating an ExtensionHost with VIEW_TYPE_EXTENSION_SIDEBAR type, it would act as a wrapper of SidebarManager.
Once we have this ExtensionViewManager, we would need to create an ExtensionSidebar following the same approach used in ExtensionPopup. Difference is that ExtensionPopup really creates a native widget (Aura in Linux, Cocoa in Mac), while here we don't need such widget. Maybe after all we don't need such ExtensionSidebar if we don't need to create a proper widget.
ltilve
commented
9 years ago The problems calling some extension APIs (in particular, asynchronous calls to extension methods, as the event listeners were properly being managed), are apparently solved by wrapping the Sidebar container as an extension host as pushed at https://github.com/ltilve/chromium/commit/3da6ddc85ac77e807d7c68730b3ec7e72fdb6622
ryumiel
commented
9 years ago WIP Patch is pushed at 3aa2d0215f4783f377effd5eb6c0b4d1777ec5d6
ryumiel
commented
9 years ago c265273f046d537bb01cf6a826e551aa3c6e4fdc fixed sidebar loading.
Pending issue is re-enable loadURL feature
ryumiel
commented
9 years ago 20dfa70a53343bfa6d8430c82d37a734b1d6395b should support window.close inside of sidebar. but it is not working as expected. I think we should follow pattens which is used in extension_popup.cc for this case.
Still working in progress
ryumiel
commented
9 years ago Fix committed at fcd532bd8278e75707c54faeaf46c0008b7934f2 in igalia-sidebar-CL-raw and igalia-sidebar-CL. It runs as expected in Mac, but the test for Linux is needed.
ryumiel
commented
9 years ago It runs as expected in Linux, too. But the unit_test was broken. It is going to be handled in #16
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.