Closed
Bug 1097479
Opened 10 years ago
Closed 9 years ago
[BrowserAPI] Allow embed remote apps or widgets in content process if nested-oop is enabled
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla40
Tracking | Status | |
---|---|---|
firefox40 | --- | fixed |
People
(Reporter: kanru, Assigned: kechen)
References
Details
Attachments
(1 file, 10 obsolete files)
Embedding apps in content process is disabled in bug 1059662. We should allow the content to embed apps if 1. Nested-oop is enabled 2. The to be embedded <iframe> is remote=true
Updated•9 years ago
|
Assignee: nobody → kechang
Comment 1•9 years ago
|
||
Hi Kan-Ru, Could you take a look at this patch? Thanks.
Attachment #8538353 -
Flags: feedback?(kchen)
Reporter | ||
Comment 2•9 years ago
|
||
Comment on attachment 8538353 [details] [diff] [review] Allow nested content process to embed apps Review of attachment 8538353 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/html/nsGenericHTMLFrameElement.cpp @@ +442,5 @@ > if (!nsIMozBrowserFrame::GetReallyIsBrowserOrApp()) { > return NS_OK; > } > > + // Only nested content process is allowed to embed an app. // Only allow content process to embed an app when nested content process is enabled. @@ +445,5 @@ > > + // Only nested content process is allowed to embed an app. > + if (XRE_GetProcessType() != GeckoProcessType_Default && > + !GetBoolAttr(nsGkAtoms::Remote) && > + !Preferences::GetBool("dom.ipc.tabs.nested.enabled", false)) { Should avoid this Preferences check by Preference cache. You could find examples in the same file like BrowserFramesEnabled() or WidgetsEnabled() The test should be IsContentProcess || !(HasRemote && NestedEnabled)
Attachment #8538353 -
Flags: feedback?(kchen)
Assignee | ||
Comment 3•9 years ago
|
||
Hi Kershaw,I would like to practice fixing some bugs. Is it okay that I take this bug? Thanks
Flags: needinfo?(kechang)
Assignee | ||
Updated•9 years ago
|
Assignee: kechang → kechen
Assignee | ||
Comment 5•9 years ago
|
||
Hi Kershaw, could you help me to check this patch? thank you.
Attachment #8541973 -
Flags: feedback?(kechang)
Comment 6•9 years ago
|
||
Comment on attachment 8541973 [details] [diff] [review] Allow nested content process to embed apps Review of attachment 8541973 [details] [diff] [review]: ----------------------------------------------------------------- Looks like your patch is mixed with something else. Please be more careful at next time. In addition, our commit messages should look like this: Bug <number> - <short description>. The description you give to the patch should generally try to describe what the patch is accomplishing, and is usually different from just the bug's title. ::: dom/html/nsGenericHTMLFrameElement.cpp @@ +444,5 @@ > } > > + // Only nested content process is allowed to embed an app. > + if (XRE_GetProcessType() != GeckoProcessType_Default && > + !GetBoolAttr(nsGkAtoms::Remote) && Please modify the comment as Kan-Ru's feedback. We also need to add a new function to check the preference. Please check BrowserFramesEnabled() or WidgetsEnabled() in the same file.
Attachment #8541973 -
Flags: feedback?(kechang) → feedback-
Comment 7•9 years ago
|
||
BTW, please also make the old patches obsolete when you upload a new one. Thanks.
Assignee | ||
Comment 8•9 years ago
|
||
Hi Kershaw, I've made some modify to the patch, could you help me to check this again? Thank you.
Attachment #8542061 -
Flags: feedback?(kechang)
Assignee | ||
Updated•9 years ago
|
Attachment #8541973 -
Attachment is obsolete: true
Assignee | ||
Updated•9 years ago
|
Attachment #8541973 -
Attachment is obsolete: false
Assignee | ||
Updated•9 years ago
|
Attachment #8541973 -
Attachment is obsolete: true
Comment 9•9 years ago
|
||
Comment on attachment 8542061 [details] [diff] [review] Allow nested content process to embed apps and add a nested pref check function Review of attachment 8542061 [details] [diff] [review]: ----------------------------------------------------------------- Please read your patch more carefully before uploading it! ::: dom/html/nsGenericHTMLFrameElement.cpp @@ +337,5 @@ > Preferences::AddBoolVarCache(&sMozWidgetsEnabled, > "dom.enable_widgets"); > } > > + return sMozWidgetsEnabled; static bool sMozWidgetsEnabled = false; This should be wrong. @@ +459,5 @@ > > + // Only allow content process to embed an app when nested content process is enabled. > + if (XRE_GetProcessType() != GeckoProcessType_Default && > + !(GetBoolAttr(nsGkAtoms::Remote) && > + NestedEnabled())){ nit: !(GetBoolAttr(nsGkAtoms::Remote) && NestedEnabled()) is better for reading.
Attachment #8542061 -
Flags: feedback?(kechang) → feedback?
Updated•9 years ago
|
Attachment #8542061 -
Flags: feedback?
Assignee | ||
Updated•9 years ago
|
Attachment #8538353 -
Attachment is obsolete: true
Assignee | ||
Updated•9 years ago
|
Attachment #8542061 -
Attachment is obsolete: true
Assignee | ||
Comment 10•9 years ago
|
||
Hi, Kershaw. I've corrected the code and fixed the nit. Could you help me to check this patch again? Thank you.
Attachment #8542470 -
Flags: feedback?(kechang)
Comment 11•9 years ago
|
||
Comment on attachment 8542470 [details] [diff] [review] Allow nested content process to embed apps and add a nested pref check function Review of attachment 8542470 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/html/nsGenericHTMLFrameElement.cpp @@ +459,5 @@ > > + // Only allow content process to embed an app when nested content process is enabled. > + if (XRE_GetProcessType() != GeckoProcessType_Default && > + !(GetBoolAttr(nsGkAtoms::Remote) && NestedEnabled())){ > + NS_WARNING("Can't embed-apps. Embed-apps is restricted to in-proc apps or when nested content process is enabled, see bug 1097479"); nit: This line is too long. We have to make it two lines.
Attachment #8542470 -
Flags: feedback?(kechang) → feedback+
Comment 12•9 years ago
|
||
> @@ +445,5 @@
> >
> > + // Only nested content process is allowed to embed an app.
> > + if (XRE_GetProcessType() != GeckoProcessType_Default &&
> > + !GetBoolAttr(nsGkAtoms::Remote) &&
> > + !Preferences::GetBool("dom.ipc.tabs.nested.enabled", false)) {
>
> Should avoid this Preferences check by Preference cache. You could find
> examples in the same file like BrowserFramesEnabled() or WidgetsEnabled()
>
> The test should be
>
> IsContentProcess || !(HasRemote && NestedEnabled)
Hi Kan-Ru,
I think the test should be IsContentProcess && !(HasRemote && NestedEnabled), right?
Could you also take a look at Kevin's patch?
Should we create another test case for this bug?
Thanks.
Updated•9 years ago
|
Attachment #8542470 -
Flags: feedback+ → feedback?(kchen)
Reporter | ||
Comment 13•9 years ago
|
||
(In reply to Kershaw Chang [:kershaw] from comment #12) > > @@ +445,5 @@ > > > > > > + // Only nested content process is allowed to embed an app. > > > + if (XRE_GetProcessType() != GeckoProcessType_Default && > > > + !GetBoolAttr(nsGkAtoms::Remote) && > > > + !Preferences::GetBool("dom.ipc.tabs.nested.enabled", false)) { > > > > Should avoid this Preferences check by Preference cache. You could find > > examples in the same file like BrowserFramesEnabled() or WidgetsEnabled() > > > > The test should be > > > > IsContentProcess || !(HasRemote && NestedEnabled) > > Hi Kan-Ru, > > I think the test should be IsContentProcess && !(HasRemote && > NestedEnabled), right? Ah, Yes. > Could you also take a look at Kevin's patch? > Should we create another test case for this bug? Yes, test case is better.
Reporter | ||
Updated•9 years ago
|
Attachment #8542470 -
Flags: feedback?(kchen) → feedback+
Assignee | ||
Comment 14•9 years ago
|
||
Hi Kershaw, I add a test. In this test I set value of preference "dom.ipc.tabs.nested.enabled" to ture and append an iframe to check if it can really open a embed remote widget. There a question, I reuse the file file_browserElement_DisallowEmbedAppInOOP.html in my test, whose filename is not really fit to the test I do. Is it okay that I use this file or I have to create another file? And please help me to check this test. Thank you.
Attachment #8545071 -
Flags: feedback?(kechang)
Assignee | ||
Updated•9 years ago
|
Attachment #8542470 -
Attachment is obsolete: true
Comment 15•9 years ago
|
||
Comment on attachment 8545071 [details] [diff] [review] 0001-Bug-1097479-Allow-nested-content-process-to-embed-ap.patch Review of attachment 8545071 [details] [diff] [review]: ----------------------------------------------------------------- We can not reuse file_browserElement_DisallowEmbedAppInOOP.html because the remote attribute is not set to true in this file. How about test_browserElement_oop_AllowEmbedAppsInNestedOOIframe.html? ::: dom/browser-element/mochitest/browserElement_AllowEmbedAppsInOOPWithPref.js @@ +6,5 @@ > +// > + > +"use strict"; > + > +SpecialPowers.setBoolPref("dom.ipc.tabs.nested.enabled", false); This should be true. @@ +14,5 @@ > + > +SpecialPowers.setAllAppsLaunchable(true); > + > +function runTest() { > + var canEmbedApp = (browserElementTestHelpers.getOOPByDefaultPref() & nit: trailing space ::: dom/html/nsGenericHTMLFrameElement.cpp @@ +456,5 @@ > if (!nsIMozBrowserFrame::GetReallyIsBrowserOrApp()) { > return NS_OK; > } > > + // Only allow content process to embed an app when nested content nit: trailing space
Attachment #8545071 -
Flags: feedback?(kechang)
Assignee | ||
Comment 16•9 years ago
|
||
Hi Kershaw, sory about the nit. I already changed the "dom.ipc.tabs.nested.enabled" and <remote> to the value true, change the file name and a file "file_browserElement_AllowEmbedAppsInOOIframe.html" Please help me to check this patch. Thank you.
Attachment #8545790 -
Flags: feedback?(kechang)
Assignee | ||
Updated•9 years ago
|
Attachment #8545071 -
Attachment is obsolete: true
Comment 17•9 years ago
|
||
Comment on attachment 8545790 [details] [diff] [review] 0001-Bug-1097479-Allow-nested-content-process-to-embed-ap.patch Review of attachment 8545790 [details] [diff] [review]: ----------------------------------------------------------------- I'd like Kan-Ru to also have a look. Just make sure this test case matches what we need. ::: dom/browser-element/mochitest/browserElement_AllowEmbedAppsInNestedOOIframe.js @@ +14,5 @@ > +SpecialPowers.setAllAppsLaunchable(true); > + > +function runTest() { > + var canEmbedApp = (browserElementTestHelpers.getOOPByDefaultPref() & > + browserElementTestHelpers._getBoolPref("dom.ipc.tabs.nested.enabled")); Use SpecialPowers.getBoolPref and there is a redundant().
Attachment #8545790 -
Flags: feedback?(kechang)
Attachment #8545790 -
Flags: feedback?(kchen)
Attachment #8545790 -
Flags: feedback+
Comment 18•9 years ago
|
||
Hi Kan-Ru, May I know your feedback about Kevin's patch? If you think it's ok, whom should we ask to review this patch?
Flags: needinfo?(kchen)
Reporter | ||
Comment 19•9 years ago
|
||
Comment on attachment 8545790 [details] [diff] [review] 0001-Bug-1097479-Allow-nested-content-process-to-embed-ap.patch Review of attachment 8545790 [details] [diff] [review]: ----------------------------------------------------------------- Looks good. Fix the comments and I'll review it. ::: dom/browser-element/mochitest/browserElement_AllowEmbedAppsInNestedOOIframe.js @@ +7,5 @@ > +"use strict"; > + > +SimpleTest.waitForExplicitFinish(); > +browserElementTestHelpers.setEnabledPref(true); > +SpecialPowers.setBoolPref("dom.ipc.tabs.nested.enabled", true); Use SpecialPowers.pushPrefEnv. There are many examples in the same directory. @@ +14,5 @@ > +SpecialPowers.setAllAppsLaunchable(true); > + > +function runTest() { > + var canEmbedApp = (browserElementTestHelpers.getOOPByDefaultPref() & > + browserElementTestHelpers._getBoolPref("dom.ipc.tabs.nested.enabled")); Use && instead of &
Attachment #8545790 -
Flags: feedback?(kchen) → feedback+
Reporter | ||
Updated•9 years ago
|
Flags: needinfo?(kchen)
Assignee | ||
Comment 20•9 years ago
|
||
Hi Kan-Ru. I took some time to trace the code. Attachment is my modification. Could you help me to review it? Thank you.
Attachment #8545790 -
Attachment is obsolete: true
Attachment #8553564 -
Flags: review?(kchen)
Reporter | ||
Comment 21•9 years ago
|
||
Comment on attachment 8553564 [details] [diff] [review] Allow nested content process-to embed apps and add a nested pref check function Review of attachment 8553564 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/browser-element/mochitest/browserElement_AllowEmbedAppsInNestedOOIframe.js @@ +9,5 @@ > +SimpleTest.waitForExplicitFinish(); > +browserElementTestHelpers.setEnabledPref(true); > +SpecialPowers.pushPrefEnv( > + {"set": [["dom.ipc.tabs.nested.enabled", true]]}, > + browserElementTestHelpers.addPermission()); This might not do what you want. browserElementTestHelpers.addPermission() is executed before pushPrefEnv. @@ +36,5 @@ > + iframe.src = 'http://example.org/tests/dom/browser-element/mochitest/file_browserElement_AllowEmbedAppsInNestedOOIframe.html'; > + }); > +} > + > +addEventListener('testready', runTest); Instead, use addEventListener('testready', () => { SpecialPowers.pushPrefEnv({"set": [["dom.ipc.tabs.nested.enabled", true]]}, runTest); });
Attachment #8553564 -
Flags: review?(kchen)
Assignee | ||
Comment 22•9 years ago
|
||
Hi, Kan-Ru. I have modified the code. Could you please help me to check this patch again? thank you.
Attachment #8553564 -
Attachment is obsolete: true
Attachment #8554416 -
Flags: review?(kchen)
Reporter | ||
Comment 23•9 years ago
|
||
Comment on attachment 8554416 [details] [diff] [review] Allow nested content process to embed apps and add a nested pref check function Review of attachment 8554416 [details] [diff] [review]: ----------------------------------------------------------------- Looks good! Remember to push to try before checkin.
Attachment #8554416 -
Flags: review?(kchen) → review+
Assignee | ||
Comment 24•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d9491263ef6d It seems my code can't pass some test cases. I will check my modification again.
Assignee | ||
Comment 25•9 years ago
|
||
Hi Kanru, when pushed the codes to the try server, I ran into some error like the following link : https://treeherder.mozilla.org/#/jobs?repo=try&revision=d9491263ef6d After discussing with Kershaw, I changed the test code to run on e10s. And the result shows that this will fix the error : https://treeherder.mozilla.org/#/jobs?repo=try&revision=070038f0ed3e And about the Marionette error, I pushed a additional try without any of my modifications. It causes error, still. So the error might be not related to my code. https://treeherder.mozilla.org/#/jobs?repo=try&revision=b09fcbc23a25 Can you help me to check this modification? Thank you.
Attachment #8554416 -
Attachment is obsolete: true
Flags: needinfo?(kchen)
Reporter | ||
Comment 26•9 years ago
|
||
Comment on attachment 8556916 [details] [diff] [review] Bug 1097479 - Allow nested content process to embed apps and add a nested pref check function Review of attachment 8556916 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/browser-element/mochitest/mochitest-oop.ini @@ +15,5 @@ > skip-if = toolkit=='gonk' || (toolkit == 'gonk' && !debug) > [test_browserElement_oop_Alert.html] > [test_browserElement_oop_AlertInFrame.html] > +[test_browserElement_oop_AllowEmbedAppsInNestedOOIframe.html] > +skip-if = !e10s mochitest-oop.ini doesn't run under e10s (under [DEFAULT] section) so this modification effectively disables this test.
Attachment #8556916 -
Flags: feedback-
Reporter | ||
Comment 27•9 years ago
|
||
The error is this line 19:43:20 INFO - JavaScript error: jar:file:///builds/slave/test/build/application/firefox/omni.ja!/components/BrowserElementParent.js, line 163: NS_ERROR_NOT_INITIALIZED: Component returned failure code: 0xc1f30001 (NS_ERROR_NOT_INITIALIZED) [nsIMessageSender.sendAsyncMessage]
Flags: needinfo?(kchen)
Assignee | ||
Updated•9 years ago
|
Assignee | ||
Comment 28•9 years ago
|
||
Hi KanRu, can you help me to review this patch? This patch is quite the same with attachment 8554416 [details] [diff] [review], but we ran into a work queue problem then(Bug 1131444), but it is fixed now. In this patch I modify the check condition to allow content process to embed an app. And also, a mochitest for this modification. However, B2G is not totally ready for supporting nested-oop, so the test case will skip it currently.
Attachment #8556916 -
Attachment is obsolete: true
Attachment #8599132 -
Flags: review?(kchen)
Assignee | ||
Comment 29•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=53665158069e There are some errors in try run. I am checking them now, but seems not related to my modification.
Reporter | ||
Comment 30•9 years ago
|
||
Comment on attachment 8599132 [details] [diff] [review] Allow nested content process to embed apps and add a nested pref check function Review of attachment 8599132 [details] [diff] [review]: ----------------------------------------------------------------- Does this test run in-process? If so, please also add a _inproc_ version. ::: dom/browser-element/mochitest/browserElement_AllowEmbedAppsInNestedOOIframe.js @@ +13,5 @@ > +SpecialPowers.setAllAppsLaunchable(true); > + > +function runTest() { > + var canEmbedApp = browserElementTestHelpers.getOOPByDefaultPref() && > + SpecialPowers.getBoolPref("dom.ipc.tabs.nested.enabled"); Looks like we could also check canEmbedApp == false when this test run in-process.
Attachment #8599132 -
Flags: review?(kchen) → review+
Assignee | ||
Comment 31•9 years ago
|
||
Carry r+ from comment 30 . Change the test condition is(e.detail.message == 'app', canEmbedApp, e.detail.message); to is(e.detail.message == 'app', true, e.detail.message); because canEmbedApp should always be true in oop situation.
Attachment #8599132 -
Attachment is obsolete: true
Attachment #8599245 -
Flags: review+
Assignee | ||
Updated•9 years ago
|
Keywords: checkin-needed
Comment 32•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/69cff2238ff9
Keywords: checkin-needed
Comment 34•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/69cff2238ff9
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox40:
--- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•