Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2126733002:
Make ScreenWakeLock ContextLifecycleObserver instead of LocalFrameLifecycleObserver (Closed)
DescriptionMake ScreenWakeLock ContextLifecycleObserver instead of LocalFrameLifecycleObserver
It is wrong to override LocalFrameLifecycleObserver::willDetachFromFrameHost
because it is not called when you expect it being called (it is not called when a page navigates).
I believe what you really want is to observe ContextLifecycleObserver::contextDestroyed,
which is called when a document is detached.
Also I don't see any reason ScreenWakeLock needs to observer didCommitLoad.
I think that observing ContextLifecycleObserver::contextDestroyed would be enough.
Currently ScreenWakeLock is the only customer of didCommitLoad.
BUG=610176
Committed: https://crrev.com/6c996c7c0c1ba8de61e4a18732b43a03315ab318
Cr-Commit-Position: refs/heads/master@{#404052}
Patch Set 1 #Patch Set 2 : temp #Patch Set 3 : temp #
Total comments: 1
Patch Set 4 : temp #
Total comments: 8
Patch Set 5 : temp #
Messages
Total messages: 33 (9 generated)
haraken@chromium.org changed reviewers: + alogvinov@yandex-team.ru, dcheng@chromium.org
PTAL
lgtm
The CQ bit was checked by haraken@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
Fixed bot failures. https://codereview.chromium.org/2126733002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/WebLocalFrameImpl.cpp (left): https://codereview.chromium.org/2126733002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/web/WebLocalFrameImpl.cpp:1492: ScreenWakeLock::provideTo(*m_frame, m_client ? m_client->serviceRegistry(): nullptr); It's too early to instantiate ScreenWakeLock here because the frame doesn't yet have a document. Moved the instantiation to ScreenWakeLock::from. https://codereview.chromium.org/2126733002/diff/60001/content/browser/wake_lo... File content/browser/wake_lock/wake_lock_browsertest.cc (left): https://codereview.chromium.org/2126733002/diff/60001/content/browser/wake_lo... content/browser/wake_lock/wake_lock_browsertest.cc:264: IN_PROC_BROWSER_TEST_F(WakeLockTest, UnlockAfterReload) { This test is testing if the wake lock is gone after didCommitLoad is called. We're replacing didCommitLoad with contextDestroyed, so the test doesn't make sense.
The CQ bit was checked by haraken@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dcheng@chromium.org Link to the patchset: https://codereview.chromium.org/2126733002/#ps60001 (title: "temp")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2126733002/diff/60001/content/browser/wake_lo... File content/browser/wake_lock/wake_lock_browsertest.cc (left): https://codereview.chromium.org/2126733002/diff/60001/content/browser/wake_lo... content/browser/wake_lock/wake_lock_browsertest.cc:264: IN_PROC_BROWSER_TEST_F(WakeLockTest, UnlockAfterReload) { On 2016/07/06 08:27:39, haraken wrote: > > This test is testing if the wake lock is gone after didCommitLoad is called. > We're replacing didCommitLoad with contextDestroyed, so the test doesn't make > sense. Hmm, isn't this still a useful thing to test? It's obvious if you look at the code, but it's not bad to explicitly check this expectation in a test too.
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
https://codereview.chromium.org/2126733002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/WebLocalFrameImpl.cpp (left): https://codereview.chromium.org/2126733002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/web/WebLocalFrameImpl.cpp:1491: if (RuntimeEnabledFeatures::wakeLockEnabled()) Sorry I may have missed something, but why is this no longer needed?
https://codereview.chromium.org/2126733002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/WebLocalFrameImpl.cpp (left): https://codereview.chromium.org/2126733002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/web/WebLocalFrameImpl.cpp:1491: if (RuntimeEnabledFeatures::wakeLockEnabled()) On 2016/07/06 08:42:17, alogvinov wrote: > Sorry I may have missed something, but why is this no longer needed? I see now, it is moved to ScreenWakeLock.
https://codereview.chromium.org/2126733002/diff/60001/content/browser/wake_lo... File content/browser/wake_lock/wake_lock_browsertest.cc (left): https://codereview.chromium.org/2126733002/diff/60001/content/browser/wake_lo... content/browser/wake_lock/wake_lock_browsertest.cc:264: IN_PROC_BROWSER_TEST_F(WakeLockTest, UnlockAfterReload) { On 2016/07/06 08:34:42, dcheng wrote: > On 2016/07/06 08:27:39, haraken wrote: > > > > This test is testing if the wake lock is gone after didCommitLoad is called. > > We're replacing didCommitLoad with contextDestroyed, so the test doesn't make > > sense. > > Hmm, isn't this still a useful thing to test? It's obvious if you look at the > code, but it's not bad to explicitly check this expectation in a test too. Reload + WaitForLoadStop doesn't detach a document, so with this CL, the line 272 fails. On the other hand, NavigateToURL detaches a document, so UnlockAfterNavigationToSelf etc passes.
On 2016/07/06 08:50:07, alogvinov wrote: > https://codereview.chromium.org/2126733002/diff/60001/third_party/WebKit/Sour... > File third_party/WebKit/Source/web/WebLocalFrameImpl.cpp (left): > > https://codereview.chromium.org/2126733002/diff/60001/third_party/WebKit/Sour... > third_party/WebKit/Source/web/WebLocalFrameImpl.cpp:1491: if > (RuntimeEnabledFeatures::wakeLockEnabled()) > On 2016/07/06 08:42:17, alogvinov wrote: > > Sorry I may have missed something, but why is this no longer needed? > > I see now, it is moved to ScreenWakeLock. Yeah, WebLocalFrameImpl::setCoreFrame is too early to instantiate ScreenWakeLock because frame.document() is not yet available.
https://codereview.chromium.org/2126733002/diff/60001/content/browser/wake_lo... File content/browser/wake_lock/wake_lock_browsertest.cc (left): https://codereview.chromium.org/2126733002/diff/60001/content/browser/wake_lo... content/browser/wake_lock/wake_lock_browsertest.cc:264: IN_PROC_BROWSER_TEST_F(WakeLockTest, UnlockAfterReload) { On 2016/07/06 08:51:23, haraken wrote: > On 2016/07/06 08:34:42, dcheng wrote: > > On 2016/07/06 08:27:39, haraken wrote: > > > > > > This test is testing if the wake lock is gone after didCommitLoad is called. > > > We're replacing didCommitLoad with contextDestroyed, so the test doesn't > make > > > sense. > > > > Hmm, isn't this still a useful thing to test? It's obvious if you look at the > > code, but it's not bad to explicitly check this expectation in a test too. > > Reload + WaitForLoadStop doesn't detach a document, so with this CL, the line > 272 fails. > > On the other hand, NavigateToURL detaches a document, so > UnlockAfterNavigationToSelf etc passes. Is the problem that we are waiting for the wrong event, i.e. load stop vs something that occurs later and does detach the document, or is the document not detached at all during reload, so that wake lock is still true after reload?
https://codereview.chromium.org/2126733002/diff/60001/content/browser/wake_lo... File content/browser/wake_lock/wake_lock_browsertest.cc (left): https://codereview.chromium.org/2126733002/diff/60001/content/browser/wake_lo... content/browser/wake_lock/wake_lock_browsertest.cc:264: IN_PROC_BROWSER_TEST_F(WakeLockTest, UnlockAfterReload) { On 2016/07/06 08:58:58, alogvinov wrote: > On 2016/07/06 08:51:23, haraken wrote: > > On 2016/07/06 08:34:42, dcheng wrote: > > > On 2016/07/06 08:27:39, haraken wrote: > > > > > > > > This test is testing if the wake lock is gone after didCommitLoad is > called. > > > > We're replacing didCommitLoad with contextDestroyed, so the test doesn't > > make > > > > sense. > > > > > > Hmm, isn't this still a useful thing to test? It's obvious if you look at > the > > > code, but it's not bad to explicitly check this expectation in a test too. > > > > Reload + WaitForLoadStop doesn't detach a document, so with this CL, the line > > 272 fails. > > > > On the other hand, NavigateToURL detaches a document, so > > UnlockAfterNavigationToSelf etc passes. > > Is the problem that we are waiting for the wrong event, i.e. load stop vs > something that occurs later and does detach the document, or is the document not > detached at all during reload, so that wake lock is still true after reload? Assuming this is doing a "real" reload, the document should definitely be detached. As a quick sanity check: - Is shell()->web_contents()->IsLoading() true after the Reload() call? - Does WaitForLoadStop return true?
https://codereview.chromium.org/2126733002/diff/60001/content/browser/wake_lo... File content/browser/wake_lock/wake_lock_browsertest.cc (left): https://codereview.chromium.org/2126733002/diff/60001/content/browser/wake_lo... content/browser/wake_lock/wake_lock_browsertest.cc:264: IN_PROC_BROWSER_TEST_F(WakeLockTest, UnlockAfterReload) { On 2016/07/06 08:58:58, alogvinov wrote: > On 2016/07/06 08:51:23, haraken wrote: > > On 2016/07/06 08:34:42, dcheng wrote: > > > On 2016/07/06 08:27:39, haraken wrote: > > > > > > > > This test is testing if the wake lock is gone after didCommitLoad is > called. > > > > We're replacing didCommitLoad with contextDestroyed, so the test doesn't > > make > > > > sense. > > > > > > Hmm, isn't this still a useful thing to test? It's obvious if you look at > the > > > code, but it's not bad to explicitly check this expectation in a test too. > > > > Reload + WaitForLoadStop doesn't detach a document, so with this CL, the line > > 272 fails. > > > > On the other hand, NavigateToURL detaches a document, so > > UnlockAfterNavigationToSelf etc passes. > > Is the problem that we are waiting for the wrong event, i.e. load stop vs > something that occurs later and does detach the document, or is the document not > detached at all during reload, so that wake lock is still true after reload? shell()->Reload(); fprintf(stderr, "%d\n", shell()->web_contents()->IsLoading()); // true fprintf(stderr, "%d\n", WaitForLoadStop(GetWebContents())); // true fprintf(stderr, "%d\n", shell()->web_contents()->IsLoading()); // false WaitForPossibleUpdate(); // Even after this call, Document::detach is not called...
On 2016/07/06 09:14:35, haraken wrote: > https://codereview.chromium.org/2126733002/diff/60001/content/browser/wake_lo... > File content/browser/wake_lock/wake_lock_browsertest.cc (left): > > https://codereview.chromium.org/2126733002/diff/60001/content/browser/wake_lo... > content/browser/wake_lock/wake_lock_browsertest.cc:264: > IN_PROC_BROWSER_TEST_F(WakeLockTest, UnlockAfterReload) { > On 2016/07/06 08:58:58, alogvinov wrote: > > On 2016/07/06 08:51:23, haraken wrote: > > > On 2016/07/06 08:34:42, dcheng wrote: > > > > On 2016/07/06 08:27:39, haraken wrote: > > > > > > > > > > This test is testing if the wake lock is gone after didCommitLoad is > > called. > > > > > We're replacing didCommitLoad with contextDestroyed, so the test doesn't > > > make > > > > > sense. > > > > > > > > Hmm, isn't this still a useful thing to test? It's obvious if you look at > > the > > > > code, but it's not bad to explicitly check this expectation in a test too. > > > > > > Reload + WaitForLoadStop doesn't detach a document, so with this CL, the > line > > > 272 fails. > > > > > > On the other hand, NavigateToURL detaches a document, so > > > UnlockAfterNavigationToSelf etc passes. > > > > Is the problem that we are waiting for the wrong event, i.e. load stop vs > > something that occurs later and does detach the document, or is the document > not > > detached at all during reload, so that wake lock is still true after reload? > > shell()->Reload(); > fprintf(stderr, "%d\n", shell()->web_contents()->IsLoading()); // true > fprintf(stderr, "%d\n", WaitForLoadStop(GetWebContents())); // true > fprintf(stderr, "%d\n", shell()->web_contents()->IsLoading()); // false > WaitForPossibleUpdate(); // Even after this call, Document::detach is not > called... We noticed that Document::detach is called on Release builds (and thus the test passes) but not called on Debug builds (and thus the test fails). Looks very strange.
On 2016/07/06 11:07:29, haraken wrote: > On 2016/07/06 09:14:35, haraken wrote: > > > https://codereview.chromium.org/2126733002/diff/60001/content/browser/wake_lo... > > File content/browser/wake_lock/wake_lock_browsertest.cc (left): > > > > > https://codereview.chromium.org/2126733002/diff/60001/content/browser/wake_lo... > > content/browser/wake_lock/wake_lock_browsertest.cc:264: > > IN_PROC_BROWSER_TEST_F(WakeLockTest, UnlockAfterReload) { > > On 2016/07/06 08:58:58, alogvinov wrote: > > > On 2016/07/06 08:51:23, haraken wrote: > > > > On 2016/07/06 08:34:42, dcheng wrote: > > > > > On 2016/07/06 08:27:39, haraken wrote: > > > > > > > > > > > > This test is testing if the wake lock is gone after didCommitLoad is > > > called. > > > > > > We're replacing didCommitLoad with contextDestroyed, so the test > doesn't > > > > make > > > > > > sense. > > > > > > > > > > Hmm, isn't this still a useful thing to test? It's obvious if you look > at > > > the > > > > > code, but it's not bad to explicitly check this expectation in a test > too. > > > > > > > > Reload + WaitForLoadStop doesn't detach a document, so with this CL, the > > line > > > > 272 fails. > > > > > > > > On the other hand, NavigateToURL detaches a document, so > > > > UnlockAfterNavigationToSelf etc passes. > > > > > > Is the problem that we are waiting for the wrong event, i.e. load stop vs > > > something that occurs later and does detach the document, or is the document > > not > > > detached at all during reload, so that wake lock is still true after reload? > > > > shell()->Reload(); > > fprintf(stderr, "%d\n", shell()->web_contents()->IsLoading()); // true > > fprintf(stderr, "%d\n", WaitForLoadStop(GetWebContents())); // true > > fprintf(stderr, "%d\n", shell()->web_contents()->IsLoading()); // false > > WaitForPossibleUpdate(); // Even after this call, Document::detach is not > > called... > > We noticed that Document::detach is called on Release builds (and thus the test > passes) but not called on Debug builds (and thus the test fails). Looks very > strange. If this cannot be fixed/worked around in this CL, I suggest disabling the test case until the problem is solved rather than completely removing it, as this is still a useful check, in fact, the spec https://www.w3.org/TR/wake-lock/ explicitly mentions the reload scenario.
On 2016/07/06 12:22:10, alogvinov wrote: > On 2016/07/06 11:07:29, haraken wrote: > > On 2016/07/06 09:14:35, haraken wrote: > > > > > > https://codereview.chromium.org/2126733002/diff/60001/content/browser/wake_lo... > > > File content/browser/wake_lock/wake_lock_browsertest.cc (left): > > > > > > > > > https://codereview.chromium.org/2126733002/diff/60001/content/browser/wake_lo... > > > content/browser/wake_lock/wake_lock_browsertest.cc:264: > > > IN_PROC_BROWSER_TEST_F(WakeLockTest, UnlockAfterReload) { > > > On 2016/07/06 08:58:58, alogvinov wrote: > > > > On 2016/07/06 08:51:23, haraken wrote: > > > > > On 2016/07/06 08:34:42, dcheng wrote: > > > > > > On 2016/07/06 08:27:39, haraken wrote: > > > > > > > > > > > > > > This test is testing if the wake lock is gone after didCommitLoad is > > > > called. > > > > > > > We're replacing didCommitLoad with contextDestroyed, so the test > > doesn't > > > > > make > > > > > > > sense. > > > > > > > > > > > > Hmm, isn't this still a useful thing to test? It's obvious if you look > > at > > > > the > > > > > > code, but it's not bad to explicitly check this expectation in a test > > too. > > > > > > > > > > Reload + WaitForLoadStop doesn't detach a document, so with this CL, the > > > line > > > > > 272 fails. > > > > > > > > > > On the other hand, NavigateToURL detaches a document, so > > > > > UnlockAfterNavigationToSelf etc passes. > > > > > > > > Is the problem that we are waiting for the wrong event, i.e. load stop vs > > > > something that occurs later and does detach the document, or is the > document > > > not > > > > detached at all during reload, so that wake lock is still true after > reload? > > > > > > shell()->Reload(); > > > fprintf(stderr, "%d\n", shell()->web_contents()->IsLoading()); // true > > > fprintf(stderr, "%d\n", WaitForLoadStop(GetWebContents())); // true > > > fprintf(stderr, "%d\n", shell()->web_contents()->IsLoading()); // false > > > WaitForPossibleUpdate(); // Even after this call, Document::detach is not > > > called... > > > > We noticed that Document::detach is called on Release builds (and thus the > test > > passes) but not called on Debug builds (and thus the test fails). Looks very > > strange. > > If this cannot be fixed/worked around in this CL, I suggest disabling the test > case until the problem is solved rather than completely removing it, as this is > still a useful check, in fact, the spec https://www.w3.org/TR/wake-lock/ > explicitly mentions the reload scenario. Thanks! dcheng@ cannot repro the problem, so this might be some config issue of my machine. Let me check it tomorrow.
On 2016/07/06 12:39:48, haraken wrote: > On 2016/07/06 12:22:10, alogvinov wrote: > > On 2016/07/06 11:07:29, haraken wrote: > > > On 2016/07/06 09:14:35, haraken wrote: > > > > > > > > > > https://codereview.chromium.org/2126733002/diff/60001/content/browser/wake_lo... > > > > File content/browser/wake_lock/wake_lock_browsertest.cc (left): > > > > > > > > > > > > > > https://codereview.chromium.org/2126733002/diff/60001/content/browser/wake_lo... > > > > content/browser/wake_lock/wake_lock_browsertest.cc:264: > > > > IN_PROC_BROWSER_TEST_F(WakeLockTest, UnlockAfterReload) { > > > > On 2016/07/06 08:58:58, alogvinov wrote: > > > > > On 2016/07/06 08:51:23, haraken wrote: > > > > > > On 2016/07/06 08:34:42, dcheng wrote: > > > > > > > On 2016/07/06 08:27:39, haraken wrote: > > > > > > > > > > > > > > > > This test is testing if the wake lock is gone after didCommitLoad > is > > > > > called. > > > > > > > > We're replacing didCommitLoad with contextDestroyed, so the test > > > doesn't > > > > > > make > > > > > > > > sense. > > > > > > > > > > > > > > Hmm, isn't this still a useful thing to test? It's obvious if you > look > > > at > > > > > the > > > > > > > code, but it's not bad to explicitly check this expectation in a > test > > > too. > > > > > > > > > > > > Reload + WaitForLoadStop doesn't detach a document, so with this CL, > the > > > > line > > > > > > 272 fails. > > > > > > > > > > > > On the other hand, NavigateToURL detaches a document, so > > > > > > UnlockAfterNavigationToSelf etc passes. > > > > > > > > > > Is the problem that we are waiting for the wrong event, i.e. load stop > vs > > > > > something that occurs later and does detach the document, or is the > > document > > > > not > > > > > detached at all during reload, so that wake lock is still true after > > reload? > > > > > > > > shell()->Reload(); > > > > fprintf(stderr, "%d\n", shell()->web_contents()->IsLoading()); // true > > > > fprintf(stderr, "%d\n", WaitForLoadStop(GetWebContents())); // true > > > > fprintf(stderr, "%d\n", shell()->web_contents()->IsLoading()); // false > > > > WaitForPossibleUpdate(); // Even after this call, Document::detach is not > > > > called... > > > > > > We noticed that Document::detach is called on Release builds (and thus the > > test > > > passes) but not called on Debug builds (and thus the test fails). Looks very > > > strange. > > > > If this cannot be fixed/worked around in this CL, I suggest disabling the test > > case until the problem is solved rather than completely removing it, as this > is > > still a useful check, in fact, the spec https://www.w3.org/TR/wake-lock/ > > explicitly mentions the reload scenario. > > Thanks! > > dcheng@ cannot repro the problem, so this might be some config issue of my > machine. Let me check it tomorrow. I thought about this more last night, and I think Document::detach is a red herring: it's almost certainly running. I wonder if the problem is IPC vs Mojo synchronization: the loading notifications are all sent over IPC atm, while wake lock has been ported to mojo. So it's inherently somewhat racy since there is no synchronization guarantees between IPC and Mojo...
On 2016/07/07 00:28:25, dcheng wrote: > On 2016/07/06 12:39:48, haraken wrote: > > On 2016/07/06 12:22:10, alogvinov wrote: > > > On 2016/07/06 11:07:29, haraken wrote: > > > > On 2016/07/06 09:14:35, haraken wrote: > > > > > > > > > > > > > > > https://codereview.chromium.org/2126733002/diff/60001/content/browser/wake_lo... > > > > > File content/browser/wake_lock/wake_lock_browsertest.cc (left): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2126733002/diff/60001/content/browser/wake_lo... > > > > > content/browser/wake_lock/wake_lock_browsertest.cc:264: > > > > > IN_PROC_BROWSER_TEST_F(WakeLockTest, UnlockAfterReload) { > > > > > On 2016/07/06 08:58:58, alogvinov wrote: > > > > > > On 2016/07/06 08:51:23, haraken wrote: > > > > > > > On 2016/07/06 08:34:42, dcheng wrote: > > > > > > > > On 2016/07/06 08:27:39, haraken wrote: > > > > > > > > > > > > > > > > > > This test is testing if the wake lock is gone after > didCommitLoad > > is > > > > > > called. > > > > > > > > > We're replacing didCommitLoad with contextDestroyed, so the test > > > > doesn't > > > > > > > make > > > > > > > > > sense. > > > > > > > > > > > > > > > > Hmm, isn't this still a useful thing to test? It's obvious if you > > look > > > > at > > > > > > the > > > > > > > > code, but it's not bad to explicitly check this expectation in a > > test > > > > too. > > > > > > > > > > > > > > Reload + WaitForLoadStop doesn't detach a document, so with this CL, > > the > > > > > line > > > > > > > 272 fails. > > > > > > > > > > > > > > On the other hand, NavigateToURL detaches a document, so > > > > > > > UnlockAfterNavigationToSelf etc passes. > > > > > > > > > > > > Is the problem that we are waiting for the wrong event, i.e. load stop > > vs > > > > > > something that occurs later and does detach the document, or is the > > > document > > > > > not > > > > > > detached at all during reload, so that wake lock is still true after > > > reload? > > > > > > > > > > shell()->Reload(); > > > > > fprintf(stderr, "%d\n", shell()->web_contents()->IsLoading()); // true > > > > > fprintf(stderr, "%d\n", WaitForLoadStop(GetWebContents())); // true > > > > > fprintf(stderr, "%d\n", shell()->web_contents()->IsLoading()); // false > > > > > WaitForPossibleUpdate(); // Even after this call, Document::detach is > not > > > > > called... > > > > > > > > We noticed that Document::detach is called on Release builds (and thus the > > > test > > > > passes) but not called on Debug builds (and thus the test fails). Looks > very > > > > strange. > > > > > > If this cannot be fixed/worked around in this CL, I suggest disabling the > test > > > case until the problem is solved rather than completely removing it, as this > > is > > > still a useful check, in fact, the spec https://www.w3.org/TR/wake-lock/ > > > explicitly mentions the reload scenario. > > > > Thanks! > > > > dcheng@ cannot repro the problem, so this might be some config issue of my > > machine. Let me check it tomorrow. > > I thought about this more last night, and I think Document::detach is a red > herring: it's almost certainly running. > > I wonder if the problem is IPC vs Mojo synchronization: the loading > notifications are all sent over IPC atm, while wake lock has been ported to > mojo. So it's inherently somewhat racy since there is no synchronization > guarantees between IPC and Mojo... Sorry, it seems I've messed up things in some way. I tried a clobber build and the issue is gone. Let me try CQ :)
The CQ bit was checked by haraken@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dcheng@chromium.org Link to the patchset: https://codereview.chromium.org/2126733002/#ps80001 (title: "temp")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from ========== Make ScreenWakeLock ContextLifecycleObserver instead of LocalFrameLifecycleObserver It is wrong to override LocalFrameLifecycleObserver::willDetachFromFrameHost because it is not called when you expect it being called (it is not called when a page navigates). I believe what you really want is to observe ContextLifecycleObserver::contextDestroyed, which is called when a document is detached. Also I don't see any reason ScreenWakeLock needs to observer didCommitLoad. I think that observing ContextLifecycleObserver::contextDestroyed would be enough. Currently ScreenWakeLock is the only customer of didCommitLoad. BUG=610176 ========== to ========== Make ScreenWakeLock ContextLifecycleObserver instead of LocalFrameLifecycleObserver It is wrong to override LocalFrameLifecycleObserver::willDetachFromFrameHost because it is not called when you expect it being called (it is not called when a page navigates). I believe what you really want is to observe ContextLifecycleObserver::contextDestroyed, which is called when a document is detached. Also I don't see any reason ScreenWakeLock needs to observer didCommitLoad. I think that observing ContextLifecycleObserver::contextDestroyed would be enough. Currently ScreenWakeLock is the only customer of didCommitLoad. BUG=610176 Committed: https://crrev.com/6c996c7c0c1ba8de61e4a18732b43a03315ab318 Cr-Commit-Position: refs/heads/master@{#404052} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/6c996c7c0c1ba8de61e4a18732b43a03315ab318 Cr-Commit-Position: refs/heads/master@{#404052} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
