1154607 - [System] make browser_context_menu as a base module

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::TV::System, defect)

Description

[John Hu [:johnhu][:johu][:醬糊小弟]]

To have sub module support, we should wrap browser_context_menu as a base module. In AppWindow, we may use BaseModule.instantiate to create the context menu.

Comment 1

[GitHub Autolander Bot]

Attachment: [gaia] huchengtw-moz:bug-1154607-browser-context-menu-as-base-module > mozilla-b2g:master

Comment 2

[John Hu [:johnhu][:johu][:醬糊小弟]]

PR is ready. Wait for all tests before requesting review.

Comment 3

[John Hu [:johnhu][:johu][:醬糊小弟]]

Comment on attachment 8594679 [details] [review]
[gaia] huchengtw-moz:bug-1154607-browser-context-menu-as-base-module > mozilla-b2g:master

This patch wraps BrowserContextMenu as a base module and use SUB_MODULE to include the ContextMenuView. The arguable part of this patch may be the initialization of SUB_MODULE in AppWindow. I had created a static variable in AppWindow called SUB_MODULE and initialized all sub modules at installSubComponent().

I had saw the patch of bug 1094759 which changes the SUB_COMPONENTS declaration from constructor object to string. If this patch is landed after bug 1094759, I would like to change the patch to detect the type of subcomponents, like:

if (window[componentName]) {
  instance = new window[componentName](app);
} else {
  instance = BaseModule.instantiate(componentName, app);
}

Please review this patch or cancel it with your thought. Thanks.

Comment 4

[(inactive after 6/18) Alive Kuo [:alive]]

Comment on attachment 8594679 [details] [review]
[gaia] huchengtw-moz:bug-1154607-browser-context-menu-as-base-module > mozilla-b2g:master

You need to change all *_window.js to add the SUB_MODULE (it's not copied to prototype)

Comment 5

[John Hu [:johnhu][:johu][:醬糊小弟]]

Oh~~~~~ You are correct. You should r- this patch. Sigh, it's my fault.

Comment 6

[John Hu [:johnhu][:johu][:醬糊小弟]]

Comment on attachment 8594679 [details] [review]
[gaia] huchengtw-moz:bug-1154607-browser-context-menu-as-base-module > mozilla-b2g:master

Alive,

Thanks for the finding. I had changed the patch to include *_window.js who have context_menu to have SUB_MODULES and to detect if a constructor has SUB_MODULES or not while install sub components.

The test cases are also changed.

Please review this patch. Thanks.

Comment 7

[(inactive after 6/18) Alive Kuo [:alive]]

Comment on attachment 8594679 [details] [review]
[gaia] huchengtw-moz:bug-1154607-browser-context-menu-as-base-module > mozilla-b2g:master

r=me

Comment 8

[GitHub Autolander Bot]

https://github.com/mozilla-b2g/gaia/pull/29595

The pull request could not be applied to the integration branch. Please try again after current integration is complete. You may need to rebase your branch against the target branch.

Comment 9

[John Hu [:johnhu][:johu][:醬糊小弟]]

clear checkin-needed to check if auto-lander is broken.

Comment 10

[John Hu [:johnhu][:johu][:醬糊小弟]]

merged to master:
https://github.com/mozilla-b2g/gaia/commit/ddb25d1e105b6a2028bd165dfac4c543fc62f955

tree herder is all green:
https://treeherder.mozilla.org/#/jobs?repo=gaia&revision=b320ab38f5615d8e26db304f6188dc87fd4f307b