Closed Bug 1271240 Opened 9 years ago Closed 9 years ago

Add prerender flag support to e10s and implement makePrerenderedBrowserActive

Categories

(Core :: DOM: Core & HTML, defect)

Product:
Core
Component:
DOM: Core & HTML
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla49
Tracking Flags:
Tracking Status
e10s - ---
firefox49 --- fixed

People

(Reporter: freesamael, Assigned: freesamael)

References

(Blocks 1 open bug)

Details

(Whiteboard: btpp-active)

Attachments

(2 files, 8 obsolete files)

Part 2: Add test . r=smaug
8.78 KB, patch
freesamael
: review+
Details | Diff | Splinter Review
Part 1: Support prerender flag in e10s and implement makePrerenderedBrowserActive(). r=smaug
19.14 KB, patch
freesamael
: review+
Details | Diff | Splinter Review
Follow up of 1061864. We should implement the e10s path for set / clear prerender flag.
Depends on: 1061864
Assignee: nobody → sawang
Attached patch Part 2: Add test case (obsolete) — — Splinter Review
Summary: Add prerender flag support to e10s → Add prerender flag support to e10s and implement makePrerenderedBrowserActive
Attachment #8750726 - Attachment is obsolete: true
Attached patch Part 2: Add test case (obsolete) — — Splinter Review
tracking-e10s: --- → ?
Whiteboard: btpp-active
new features that haven't shipped aren't tracked by the e10s team. it's expected they should be e10s compat before enabled.
tracking-e10s: ? → -
Comment on attachment 8750725 [details] [diff] [review] Part 1: Support prerender flag in e10s and implement makePrerenderedBrowserActive() Hi Olli, Since the implementation of cross-docshell session history will be relatively complex, I split the code of setting / clearing prerender flag into this new bug so that I can implement PageVisibility API (bug 1069772) first before bug 1085045 is implemented. This patch addressed one issue you mentioned in bug 1085045 comment 8 - it doesn't use TabParent::SendSetIsPrerendered(), but adding a member in TabContext instead, and it uses makePrerenderedBrowserActive() to clear prerender flag. Could you help to review this patch?
Attachment #8750725 - Flags: review?(bugs)
Attachment #8750729 - Attachment is obsolete: true
Comment on attachment 8751101 [details] [diff] [review] Part 2: Add test case. r=smaug Hi Olli, could you help to review the test case? It's an e10s only test case, but it also tests non-remote <xul:browser>.
Attachment #8751101 - Flags: review?(bugs)
Attachment #8751181 - Attachment is obsolete: true
Comment on attachment 8750725 [details] [diff] [review] Part 1: Support prerender flag in e10s and implement makePrerenderedBrowserActive() Update patch...
Attachment #8750725 - Flags: review?(bugs)
Attachment #8750725 - Attachment is obsolete: true
Comment on attachment 8751183 [details] [diff] [review] Part 1: Support prerender flag in e10s and implement makePrerenderedBrowserActive() I got second thought that I should just make nsDocShell::mIsPrerendered being cleared when nsDocShell::SetIsActive() is invoked since docshell shouldn't be "prerendered" and "active" at the same time. I updated the patch accordingly. Could you help to review the patch based on the description in comment 5?
Attachment #8751183 - Flags: review?(bugs)
Comment on attachment 8751183 [details] [diff] [review] Part 1: Support prerender flag in e10s and implement makePrerenderedBrowserActive() >+++ b/dom/ipc/TabContext.cpp Shouldn't TabContext::TabContext() initialize mIsPrerendered to false.
Attachment #8751183 - Flags: review?(bugs) → review+
Attachment #8751101 - Flags: review?(bugs) → review+
Attachment #8752050 - Attachment description: Part 1: Support prerender flag in e10s and implement makePrerenderedBrowserActive() → Part 1: Support prerender flag in e10s and implement makePrerenderedBrowserActive(). r=smaug
Attachment #8752050 - Flags: review+
Attachment #8751183 - Attachment is obsolete: true
Attachment #8751101 - Attachment description: Part 2: Add test case → Part 2: Add test case. r=smaug
try to re-set checkin-needed again, hopefully it gets land
Keywords: checkin-needed
mochitest-chrome doesn't run in e10s mode, so Part 2 is effectively a no-op at the moment. Was that intentional?
Flags: needinfo?(sawang)
Keywords: checkin-needed
Comment on attachment 8753663 [details] [diff] [review] Part 2: Add test case. r=smaug split the test case into 2 separate tests
Attachment #8753663 - Attachment description: Part 2: Add test case → Part 2: Add test case. r=smaug
Attachment #8753663 - Flags: review+
Attachment #8751101 - Attachment is obsolete: true
Flags: needinfo?(sawang)
(In reply to Ryan VanderMeulen [:RyanVM] from comment #16) > mochitest-chrome doesn't run in e10s mode, so Part 2 is effectively a no-op > at the moment. Was that intentional? I wasn't aware of that try server doesn't run mochitest-chrome in e10s. I was running it locally in e10s mode. Now I split the test case into 2, so at least the non-remote test will be run on try server.
This has nothing to do with Try. Even locally, mochitest-chrome runs tests in the parent process, so e10s mode isn't testing what you think.
I'm lost now. The test explicitly does <browser remote=true>, so why wouldn't it be testing e10s?
(In reply to Ryan VanderMeulen [:RyanVM] from comment #21) > This has nothing to do with Try. Even locally, mochitest-chrome runs tests > in the parent process, so e10s mode isn't testing what you think. When I first read the test, I thought this too; however, the test actually does create a remote browser, which does work in e10s (whether or not --e10s is passed to mochitest-chrome). This is equivalent to the mochitest-plain tests that create a mozbrowser iframe in order to specially interact with its message manager and test e10s.
Thanks for clarifying everyone, sorry for the confusion on my part.
What's with all the e10s test_scopes.html failures on that Try push, though? I don't see those on the Treeherder results for the parent commit. https://treeherder.mozilla.org/logviewer.html#?job_id=21008605&repo=try#L3126
The other run yesterday which also includes the patches doesn't seem to have the same failure: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e87e8bd0d78fdb9afa9ac21507b3857224d16993 I'm trying another run on mochitests...
(In reply to Samael Wang [:freesamael][:sawang] from comment #26) > https://treeherder.mozilla.org/#/jobs?repo=try&revision=0f1af03c7b92 This run on mochitests is all greeen.
Part 1 needs rebasing :\
Keywords: checkin-needed
rebase patch, just in case
Attachment #8752050 - Attachment is obsolete: true
Attachment #8753663 - Attachment is obsolete: true
Attachment #8754645 - Attachment description: Part 1: Support prerender flag in e10s and implement makePrerenderedBrowserActive() → Part 1: Support prerender flag in e10s and implement makePrerenderedBrowserActive(). r=smaug
Attachment #8754645 - Flags: review+
Attachment #8754638 - Attachment description: Part 2: Add test case → Part 2: Add test . r=smaug
Attachment #8754638 - Flags: review+
Both patches are rebased to tip of m-c.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/91bf876e9f32 https://hg.mozilla.org/mozilla-central/rev/39b01a2c5fbe
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox49: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: