Closed Bug 1154607 Opened 7 years ago Closed 7 years ago

[System] make browser_context_menu as a base module

Categories

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

x86
macOS
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: johnhu, Assigned: johnhu)

References

Details

Attachments

(1 file)

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.
No longer blocks: system-tv-visual
Assignee: nobody → im
PR is ready. Wait for all tests before requesting review.
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.
Attachment #8594679 - Flags: review?(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)
Attachment #8594679 - Flags: review?(alive) → feedback+
Oh~~~~~ You are correct. You should r- this patch. Sigh, it's my fault.
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.
Attachment #8594679 - Flags: review?(alive)
Comment on attachment 8594679 [details] [review]
[gaia] huchengtw-moz:bug-1154607-browser-context-menu-as-base-module > mozilla-b2g:master

r=me
Attachment #8594679 - Flags: review?(alive) → review+
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.
Whiteboard: checkin-needed
clear checkin-needed to check if auto-lander is broken.
Whiteboard: checkin-needed
You need to log in before you can comment on or make changes to this bug.