Issue 18777003: Extract simpler classes for observing context lifecycle and observe Page lifecycle inNavigatorVibra…

Issue 18777003: Extract simpler classes for observing context lifecycle and observe Page lifecycle inNavigatorVibra… (Closed)

Created:
7 years, 5 months ago by Michael van Ouwerkerk

Modified:
7 years, 5 months ago

Reviewers:
abarth-chromium, loislo

CC:
blink-reviews, dglazkov+blink, eae+blinkwatch, adamk+blink_chromium.org

Base URL:
https://chromium.googlesource.com/chromium/blink.git@master

Visibility:
Public.

More Reviews

Description

Extract a minimal set of classes for observing the lifecycle of a context. From ContextLifecycleObserver extract LifecycleObserver. From ContextLifecycleNotifier extract LifecycleNotifier. From ScriptExecutionContext extract LifecycleContext. Use these minimal classes to observe Page. In this case it is used to observe page visibility in the Vibration API. When the page is no longer visible all running vibrations must be cancelled. BUG=222504 R=abarth@chromium.org, loislo@chromium.org Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=154401

Patch Set 1 #

Patch Set 2 : Page inherits from the new SimpleLifecycleContext. Define and use PageLifecycleObserver. #

Patch Set 3 : Use SimpleLifecycleObserver in addObserver / removeObserver. #

Total comments: 4

Patch Set 4 : Drop the 'Simpler'. It's cleaner. #

Patch Set 5 : Ensure m_scriptExecutionContext is maintained and not just null. #

Total comments: 10

Patch Set 6 : Move Lifecycle files from dom/ to platform/ and add a page getter to PageLifecycleObserver. #

Patch Set 7 : Rebase and skip nmi-webaudio tests. #

Patch Set 8 : Rebase. #

Patch Set 9 : rebaselined~ #

Created: 7 years, 5 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+109 lines, -116 lines)			Patch
M	Source/core/core.gypi	View	1 2 3 4 5 6 7 8	2 chunks	+10 lines, -0 lines	0 comments	Download
M	Source/core/dom/ContextLifecycleNotifier.h	View	1 2 3 4 5 6 7 8	4 chunks	+4 lines, -17 lines	0 comments	Download
M	Source/core/dom/ContextLifecycleNotifier.cpp	View	1 2 3 4 5 6 7 8	5 chunks	+14 lines, -22 lines	0 comments	Download
M	Source/core/dom/ContextLifecycleObserver.h	View	1 2 3 4 5	1 chunk	+6 lines, -8 lines	0 comments	Download
M	Source/core/dom/ContextLifecycleObserver.cpp	View	1 2 3 4	3 chunks	+5 lines, -1 line	0 comments	Download
M	Source/core/dom/Document.h	View	1 2 3 4 5 6 7 8	1 chunk	+1 line, -1 line	0 comments	Download
M	Source/core/dom/Document.cpp	View	1 2 3 4 5 6 7 8	1 chunk	+1 line, -1 line	0 comments	Download
M	Source/core/dom/DocumentLifecycleObserver.h	View	1 2 3	1 chunk	+3 lines, -3 lines	0 comments	Download
M	Source/core/dom/DocumentLifecycleObserver.cpp	View	1 2 3	1 chunk	+6 lines, -6 lines	0 comments	Download
M	Source/core/dom/ScriptExecutionContext.h	View	1 2 3 4 5 6 7 8	5 chunks	+3 lines, -7 lines	0 comments	Download
M	Source/core/dom/ScriptExecutionContext.cpp	View	1 2 3 4 5 6 7 8	2 chunks	+2 lines, -14 lines	0 comments	Download
M	Source/core/page/Page.h	View	1 2 3 4 5 6 7 8	5 chunks	+8 lines, -1 line	0 comments	Download
M	Source/core/page/Page.cpp	View	1 2 3 4 5 6 7 8	3 chunks	+14 lines, -0 lines	0 comments	Download
M	Source/modules/vibration/NavigatorVibration.h	View	1 2 3 4 5	1 chunk	+13 lines, -9 lines	0 comments	Download
M	Source/modules/vibration/NavigatorVibration.cpp	View	1 2 3 4 5 6	5 chunks	+19 lines, -26 lines	0 comments	Download

Messages

Total messages: 24 (0 generated)

Expand Messages | Collapse Messages

Michael van Ouwerkerk

Hi Adam, this is a followup on the discussion we had earlier about how to ...

7 years, 5 months ago (2013-07-08 16:43:27 UTC) #1

abarth-chromium

Would you be willing to add more information to the description? What you've got there ...

7 years, 5 months ago (2013-07-08 21:16:43 UTC) #2

abarth-chromium

https://codereview.chromium.org/18777003/diff/5001/Source/core/core.gypi File Source/core/core.gypi (right): https://codereview.chromium.org/18777003/diff/5001/Source/core/core.gypi#newcode1916 Source/core/core.gypi:1916: 'dom/SimpleLifecycleObserver.h', The word "Simple" in a name is usually ...

7 years, 5 months ago (2013-07-08 21:25:24 UTC) #3

Michael van Ouwerkerk

Thanks for the quick review Adam! I've expanded the description and hopefully sufficiently addressed your ...

7 years, 5 months ago (2013-07-09 17:09:11 UTC) #4

Thanks for the quick review Adam! I've expanded the description and hopefully
sufficiently addressed your concerns. Please take another look.

https://codereview.chromium.org/18777003/diff/5001/Source/core/core.gypi
File Source/core/core.gypi (right):

https://codereview.chromium.org/18777003/diff/5001/Source/core/core.gypi#newc...
Source/core/core.gypi:1916: 'dom/SimpleLifecycleObserver.h',
On 2013/07/08 21:25:24, abarth wrote:
> The word "Simple" in a name is usually a bad sign.  In what way is it simple? 
> Can we just drop the name (e.g., LifecycleObserver rather than
> SimpleLifecycleObserver)?

Agreed it's not a good naming practice. These were kind of placeholder names
while I mulled repurposing the ContextLifecycle* names. Just Lifecycle* looks ok
though. Done.

https://codereview.chromium.org/18777003/diff/5001/Source/core/dom/SimpleLife...
File Source/core/dom/SimpleLifecycleNotifier.h (right):

https://codereview.chromium.org/18777003/diff/5001/Source/core/dom/SimpleLife...
Source/core/dom/SimpleLifecycleNotifier.h:60: IteratingOverPageObservers
The file branch detection may be confusing here - the system thinks the original
file was ContextLifecycleObserver.h. The real original is
ContextLifecycleNotifier.h and it contains a lot of code for managing
ActiveDOMObjects. I don't think that's needed in the 'simple' use case, and I
didn't want to make that functionality and its dependencies part of the Page
class.

The enum that knows about all subclasses seems like an anti pattern. However,
the enum was already there in ContextLifecycleNotifier, I didn't add that to
this system:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

The addObserver and removeObserver methods were already virtual, I didn't add
that generality:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

Really, in this patch I'm trying to avoid pulling too much complexity into the
Page class. The alternative would be to not extract these simple base classes
and just make Page inherit from ScriptExecutionContext, which is quite large and
does much more than some basic lifecycle observation.

On 2013/07/08 21:25:24, abarth wrote:
> I'm not sure it's worth abstracting the SimpleLifecycleNotifier base class.  I
> would just create a PageLifecycleNotifier and not worry too much about the
five
> duplicated lines of code.
> 
> The downside to the approach in this CL is that you have the enum that
requires
> this class to know about all of its subclasses.  That introduces some
non-sense
> states like a DocumentLifecycleNotifier be in the IteratingOverPageObservers
> state (at least according to the type system).
> 
> I can see how a SimpleLifecycleNotifier would make sense if it didn't need to
> know about all of its subclasses...  Even then, though, it looks like you've
> made addObserver and removeObserver virtual, which seems like more
unnecessarily
> generality....

abarth-chromium

On 2013/07/09 17:09:11, Michael van Ouwerkerk wrote: > The alternative would be to not extract ...

7 years, 5 months ago (2013-07-09 18:18:44 UTC) #5

abarth-chromium

LGTM with the comments below addressed. Thanks for updating the diff. This version made it ...

7 years, 5 months ago (2013-07-09 18:27:52 UTC) #6

Michael van Ouwerkerk

Thanks Adam! https://codereview.chromium.org/18777003/diff/15001/Source/core/core.gypi File Source/core/core.gypi (right): https://codereview.chromium.org/18777003/diff/15001/Source/core/core.gypi#newcode1825 Source/core/core.gypi:1825: 'dom/LifecycleObserver.h', On 2013/07/09 18:27:52, abarth wrote: > ...

7 years, 5 months ago (2013-07-10 13:25:09 UTC) #7

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mvanouwerkerk@chromium.org/18777003/26001

7 years, 5 months ago (2013-07-10 13:26:05 UTC) #8

commit-bot: I haz the power

Retried try job too often on linux_layout_rel for step(s) webkit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_layout_rel&number=15824

7 years, 5 months ago (2013-07-10 14:42:00 UTC) #9

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mvanouwerkerk@chromium.org/18777003/26001

7 years, 5 months ago (2013-07-10 14:50:29 UTC) #10

commit-bot: I haz the power

Retried try job too often on linux_layout_rel for step(s) webkit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_layout_rel&number=15838

7 years, 5 months ago (2013-07-10 15:36:18 UTC) #11

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mvanouwerkerk@chromium.org/18777003/26001

7 years, 5 months ago (2013-07-10 15:40:04 UTC) #12

commit-bot: I haz the power

Retried try job too often on linux_layout_rel for step(s) webkit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_layout_rel&number=15856

7 years, 5 months ago (2013-07-10 16:47:38 UTC) #13

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mvanouwerkerk@chromium.org/18777003/26001

7 years, 5 months ago (2013-07-11 13:17:16 UTC) #14

commit-bot: I haz the power

Retried try job too often on linux_layout_rel for step(s) webkit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_layout_rel&number=16168

7 years, 5 months ago (2013-07-11 14:04:56 UTC) #15

abarth-chromium

Looks like there's a problem with these tests: nmi-webaudio-leak-test.html nmi-webaudio.html

7 years, 5 months ago (2013-07-11 17:18:30 UTC) #16

Michael van Ouwerkerk

Ilya, could you please review the additions to TestExpectations to confirm this is as intended? ...

7 years, 5 months ago (2013-07-16 15:35:29 UTC) #17

loislo

On 2013/07/16 15:35:29, Michael van Ouwerkerk wrote: > Ilya, could you please review the additions ...

7 years, 5 months ago (2013-07-16 18:10:44 UTC) #18

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mvanouwerkerk@chromium.org/18777003/61001

7 years, 5 months ago (2013-07-17 00:39:54 UTC) #19

commit-bot: I haz the power

Failed to apply patch for LayoutTests/TestExpectations: While running patch -p1 --forward --force --no-backup-if-mismatch; patching file ...

7 years, 5 months ago (2013-07-17 00:40:06 UTC) #20

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mvanouwerkerk@chromium.org/18777003/68001

7 years, 5 months ago (2013-07-17 13:27:20 UTC) #21

commit-bot: I haz the power

Failed to apply patch for Source/core/dom/ContextLifecycleNotifier.cpp: While running patch -p1 --forward --force --no-backup-if-mismatch; patching file ...

7 years, 5 months ago (2013-07-17 13:27:28 UTC) #22

Failed to apply patch for Source/core/dom/ContextLifecycleNotifier.cpp:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file Source/core/dom/ContextLifecycleNotifier.cpp
  Hunk #1 FAILED at 28.
  Hunk #2 succeeded at 77 (offset -1 lines).
  Hunk #3 succeeded at 88 (offset -1 lines).
  Hunk #4 succeeded at 99 (offset -1 lines).
  Hunk #5 succeeded at 110 (offset -1 lines).
  1 out of 5 hunks FAILED -- saving rejects to file
Source/core/dom/ContextLifecycleNotifier.cpp.rej

Patch:       Source/core/dom/ContextLifecycleNotifier.cpp
Index: Source/core/dom/ContextLifecycleNotifier.cpp
diff --git a/Source/core/dom/ContextLifecycleNotifier.cpp
b/Source/core/dom/ContextLifecycleNotifier.cpp
index
1c1b72f28d7ce8cef7b62ecce9954e34a58bf611..e603efd574f9ee370fa970b5907a6576506ba606
100644
--- a/Source/core/dom/ContextLifecycleNotifier.cpp
+++ b/Source/core/dom/ContextLifecycleNotifier.cpp
@@ -28,46 +28,38 @@
 #include "config.h"
 #include "core/dom/ContextLifecycleNotifier.h"
 
+#include "core/dom/ScriptExecutionContext.h"
 #include "core/dom/WebCoreMemoryInstrumentation.h"
 #include "wtf/TemporaryChange.h"
 
 namespace WebCore {
 
 ContextLifecycleNotifier::ContextLifecycleNotifier(ScriptExecutionContext*
context)
-    : m_context(context)
-    , m_iterating(IteratingNone)
-    , m_inDestructor(false)
+    : LifecycleNotifier(context)
 {
 }
 
 ContextLifecycleNotifier::~ContextLifecycleNotifier()
 {
-    m_inDestructor = true;
-    for (ContextObserverSet::iterator iter = m_contextObservers.begin(); iter
!= m_contextObservers.end(); iter = m_contextObservers.begin()) {
-        ContextLifecycleObserver* observer = *iter;
-        m_contextObservers.remove(observer);
-        ASSERT(observer->scriptExecutionContext() == m_context);
-        observer->contextDestroyed();
-    }
 }
 
-void ContextLifecycleNotifier::addObserver(ContextLifecycleObserver* observer,
ContextLifecycleObserver::Type as)
+void ContextLifecycleNotifier::addObserver(LifecycleObserver* observer,
LifecycleObserver::Type type)
 {
-    RELEASE_ASSERT(!m_inDestructor);
+    LifecycleNotifier::addObserver(observer, type);
+
     RELEASE_ASSERT(m_iterating != IteratingOverContextObservers);
-    m_contextObservers.add(observer);
-    if (as == ContextLifecycleObserver::ActiveDOMObjectType) {
+    if (type == LifecycleObserver::ActiveDOMObjectType) {
         RELEASE_ASSERT(m_iterating != IteratingOverActiveDOMObjects);
         m_activeDOMObjects.add(static_cast<ActiveDOMObject*>(observer));
     }
 }
 
-void ContextLifecycleNotifier::removeObserver(ContextLifecycleObserver*
observer, ContextLifecycleObserver::Type as)
+void ContextLifecycleNotifier::removeObserver(LifecycleObserver* observer,
LifecycleObserver::Type type)
 {
-    RELEASE_ASSERT(!m_inDestructor);
+    LifecycleNotifier::removeObserver(observer, type);
+
     RELEASE_ASSERT(m_iterating != IteratingOverContextObservers);
-    m_contextObservers.remove(observer);
-    if (as == ContextLifecycleObserver::ActiveDOMObjectType) {
+    if (type == LifecycleObserver::ActiveDOMObjectType) {
         RELEASE_ASSERT(m_iterating != IteratingOverActiveDOMObjects);
         m_activeDOMObjects.remove(static_cast<ActiveDOMObject*>(observer));
     }
@@ -78,7 +70,7 @@ void
ContextLifecycleNotifier::notifyResumingActiveDOMObjects()
     TemporaryChange<IterationType> scope(this->m_iterating,
IteratingOverActiveDOMObjects);
     ActiveDOMObjectSet::iterator activeObjectsEnd = m_activeDOMObjects.end();
     for (ActiveDOMObjectSet::iterator iter = m_activeDOMObjects.begin(); iter
!= activeObjectsEnd; ++iter) {
-        ASSERT((*iter)->scriptExecutionContext() == m_context);
+        ASSERT((*iter)->scriptExecutionContext() == context());
         ASSERT((*iter)->suspendIfNeededCalled());
         (*iter)->resume();
     }
@@ -89,7 +81,7 @@ void
ContextLifecycleNotifier::notifySuspendingActiveDOMObjects(ActiveDOMObject:
     TemporaryChange<IterationType> scope(this->m_iterating,
IteratingOverActiveDOMObjects);
     ActiveDOMObjectSet::iterator activeObjectsEnd = m_activeDOMObjects.end();
     for (ActiveDOMObjectSet::iterator iter = m_activeDOMObjects.begin(); iter
!= activeObjectsEnd; ++iter) {
-        ASSERT((*iter)->scriptExecutionContext() == m_context);
+        ASSERT((*iter)->scriptExecutionContext() == context());
         ASSERT((*iter)->suspendIfNeededCalled());
         (*iter)->suspend(why);
     }
@@ -100,7 +92,7 @@ void
ContextLifecycleNotifier::notifyStoppingActiveDOMObjects()
     TemporaryChange<IterationType> scope(this->m_iterating,
IteratingOverActiveDOMObjects);
     ActiveDOMObjectSet::iterator activeObjectsEnd = m_activeDOMObjects.end();
     for (ActiveDOMObjectSet::iterator iter = m_activeDOMObjects.begin(); iter
!= activeObjectsEnd; ++iter) {
-        ASSERT((*iter)->scriptExecutionContext() == m_context);
+        ASSERT((*iter)->scriptExecutionContext() == context());
         ASSERT((*iter)->suspendIfNeededCalled());
         (*iter)->stop();
     }
@@ -111,7 +103,7 @@ bool ContextLifecycleNotifier::canSuspendActiveDOMObjects()
     TemporaryChange<IterationType> scope(this->m_iterating,
IteratingOverActiveDOMObjects);
     ActiveDOMObjectSet::iterator activeObjectsEnd = m_activeDOMObjects.end();
     for (ActiveDOMObjectSet::const_iterator iter = m_activeDOMObjects.begin();
iter != activeObjectsEnd; ++iter) {
-        ASSERT((*iter)->scriptExecutionContext() == m_context);
+        ASSERT((*iter)->scriptExecutionContext() == context());
         ASSERT((*iter)->suspendIfNeededCalled());
         if (!(*iter)->canSuspend())
             return false;

loislo

Committed patchset #9 manually as r154401 (presubmit successful).

7 years, 5 months ago (2013-07-17 14:21:48 UTC) #23

loislo

7 years, 5 months ago (2013-07-17 14:48:40 UTC) #24

Message was sent while issue was closed.

On 2013/07/17 14:21:48, loislo wrote:
> Committed patchset #9 manually as r154401 (presubmit successful).

New files were landed as r154402.

Expand Messages | Collapse Messages