1271240 - Add prerender flag support to e10s and implement makePrerenderedBrowserActive

Status: RESOLVED FIXED

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

Description

[Samael Wang [:freesamael] (away for now)]

Follow up of 1061864. We should implement the e10s path for set / clear prerender flag.

Comment 1

[Samael Wang [:freesamael] (away for now)]

Attachment: Part 1: Support prerender flag in e10s and implement makePrerenderedBrowserActive()

Comment 2

[Samael Wang [:freesamael] (away for now)]

Attachment: Part 2: Add test case

Comment 3

[Samael Wang [:freesamael] (away for now)]

Attachment: Part 2: Add test case

Comment 4

[Jim Mathies [:jimm]]

new features that haven't shipped aren't tracked by the e10s team. it's expected they should be e10s compat before enabled.

Comment 5

[Samael Wang [:freesamael] (away for now)]

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?

Comment 6

[Samael Wang [:freesamael] (away for now)]

Attachment: Part 2: Add test case. r=smaug

Comment 7

[Samael Wang [:freesamael] (away for now)]

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>.

Comment 8

[Samael Wang [:freesamael] (away for now)]

Attachment: Part 1: Support prerender flag in e10s and implement makePrerenderedBrowserActive()

Comment 9

[Samael Wang [:freesamael] (away for now)]

Attachment: Part 1: Support prerender flag in e10s and implement makePrerenderedBrowserActive()

Comment 10

[Samael Wang [:freesamael] (away for now)]

Comment on attachment 8750725 [details] [diff] [review]
Part 1: Support prerender flag in e10s and implement makePrerenderedBrowserActive()

Update patch...

Comment 11

[Samael Wang [:freesamael] (away for now)]

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?

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 13

[Samael Wang [:freesamael] (away for now)]

Attachment: Part 1: Support prerender flag in e10s and implement makePrerenderedBrowserActive(). r=smaug

Fix nits Olli mentioned.

Comment 14

[Samael Wang [:freesamael] (away for now)]

Running try job: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a88abee7d56c1bff99cfe09d0dd18a16d35a5ad0

Comment 15

[Samael Wang [:freesamael] (away for now)]

try to re-set checkin-needed again, hopefully it gets land

Comment 16

[Ryan VanderMeulen [:RyanVM]]

mochitest-chrome doesn't run in e10s mode, so Part 2 is effectively a no-op at the moment. Was that intentional?

Comment 17

[Samael Wang [:freesamael] (away for now)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=461eef435893

Comment 18

[Samael Wang [:freesamael] (away for now)]

Attachment: Part 2: Add test case. r=smaug

Comment 19

[Samael Wang [:freesamael] (away for now)]

Comment on attachment 8753663 [details] [diff] [review]
Part 2: Add test case. r=smaug

split the test case into 2 separate tests

Comment 20

[Samael Wang [:freesamael] (away for now)]

(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.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 22

[Olli Pettay [:smaug][[e-mail]]]

I'm lost now. The test explicitly does <browser remote=true>, so why wouldn't it be testing e10s?

Comment 23

[Blake Kaplan (:mrbkap) (inactive)]

(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.

Comment 24

[Ryan VanderMeulen [:RyanVM]]

Thanks for clarifying everyone, sorry for the confusion on my part.

Comment 25

[Ryan VanderMeulen [:RyanVM]]

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

Comment 26

[Samael Wang [:freesamael] (away for now)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0f1af03c7b92

Comment 27

[Samael Wang [:freesamael] (away for now)]

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...

Comment 28

[Samael Wang [:freesamael] (away for now)]

(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.

Comment 29

[Ryan VanderMeulen [:RyanVM]]

Part 1 needs rebasing :\

Comment 30

[Samael Wang [:freesamael] (away for now)]

Attachment: Part 2: Add test . r=smaug

rebase patch, just in case

Comment 31

[Samael Wang [:freesamael] (away for now)]

Attachment: Part 1: Support prerender flag in e10s and implement makePrerenderedBrowserActive(). r=smaug

rebase part 1

Comment 32

[Samael Wang [:freesamael] (away for now)]

Both patches are rebased to tip of m-c.

Comment 33

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/91bf876e9f32
https://hg.mozilla.org/integration/mozilla-inbound/rev/39b01a2c5fbe

Comment 34

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/91bf876e9f32
https://hg.mozilla.org/mozilla-central/rev/39b01a2c5fbe