Bug 194977 – Introduce ContentChangeObserver class

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[zalan]

This class will eventually take care of all hover related content change observation logic.

[Radar WebKit Bug Importer]

<rdar://problem/48338115>

[zalan]

Created attachment 362829 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 362829 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=362829&action=review

> Source/WebCore/page/DOMTimer.cpp:242
> +        LOG_WITH_STREAM(ContentObservation, stream << "DOMTimer::install: registed this timer: (" << timer << ") and observe when it fires.");

registed. Maybe log the timer ID?

> Source/WebCore/page/DOMTimer.cpp:360
> +    if (is<Document>(context) && downcast<Document>(context).page()) {
> +        page = downcast<Document>(context).page();
> +        auto& contentChangeObserver = page->contentChangeObserver();
> +        isObservingLastTimer = contentChangeObserver.countOfObservedDOMTimers() == 1;
> +        shouldBeginObservingChanges = contentChangeObserver.containsObservedDOMTimer(*this);
>      }

If this were a lambda the code would read more nicely.

> Source/WebCore/page/DOMTimer.cpp:391
>              LOG(ContentObservation, "DOMTimer::fired: in determined state.");

Log the timer ID?

> Source/WebCore/page/DOMTimer.cpp:395
>              LOG(ContentObservation, "DOMTimer::fired: wait until next style recalc fires.");

Ditto.

> Source/WebCore/page/DOMWindow.cpp:1698
> +        LOG_WITH_STREAM(ContentObservation, stream << "DOMWindow::clearTimeout: remove registered timer (" << timer << ")");

Log the ID

> Source/WebCore/page/DOMWindow.cpp:1704
> +    handleObservedTimerCancelIfNeeded();

The lambda doesn't really add anything here.

> Source/WebCore/page/ios/EventHandlerIOS.mm:495
> +    document.updateStyleIfNeeded();

Would be nice to add a comment to say why we have do this here and again just below.

[zalan]

Created attachment 362882 [details]
Patch

[zalan]

(In reply to Simon Fraser (smfr) from comment #3)
> > Source/WebCore/page/DOMWindow.cpp:1704
> > +    handleObservedTimerCancelIfNeeded();
> 
> The lambda doesn't really add anything here.
I think it does. It enables a nicer looking control flow.

if () {
    if () {
        if () {
            if () {
                do();
            }
        }
    }
}
vs.
if (!)
    return;
if (!)
    return;
if (!)
    return;
if (!)
    return;
do()

[WebKit Commit Bot]

Comment on attachment 362882 [details]
Patch

Clearing flags on attachment: 362882

Committed r242032: <https://trac.webkit.org/changeset/242032>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Frédéric Wang (:fredw)]

@Zalan: This is making iOS debug build fails: https://build.webkit.org/builders/Apple%20iOS%2012%20Simulator%20Debug%20%28Build%29

n file included from /Users/fred/WebKit/WebKitBuild/Debug-iphonesimulator/DerivedSources/WebCore/unified-sources/UnifiedSource315.cpp:5:
./page/DOMTimer.cpp:242:102: error: invalid use of member 'm_timeoutId' in
      static member function
  ...<< "DOMTimer::install: register this timer: (" << m_timeoutId << ") and ...
                                                       ^~~~~~~~~~~
In file included from /Users/fred/WebKit/WebKitBuild/Debug-iphonesimulator/DerivedSources/WebCore/unified-sources/UnifiedSource315.cpp:5:
In file included from ./page/DOMTimer.cpp:32:
In file included from /Users/fred/WebKit/Source/WebCore/platform/Logging.h:28:
PAL/pal/LogMacros.h:36:9: note: expanded from macro 'LOG_WITH_STREAM'
        commands; \
        ^~~~~~~~
In file included from /Users/fred/WebKit/WebKitBuild/Debug-iphonesimulator/DerivedSources/WebCore/unified-sources/UnifiedSource315.cpp:5:
./page/DOMTimer.cpp:391:68: error: invalid operands to binary expression
      ('const char *' and 'int')
  ...LOG_WITH_STREAM(ContentObservation, "DOMTimer::fired(" << m_timeoutId <...
                                         ~~~~~~~~~~~~~~~~~~ ^  ~~~~~~~~~~~
In file included from /Users/fred/WebKit/WebKitBuild/Debug-iphonesimulator/DerivedSources/WebCore/unified-sources/UnifiedSource315.cpp:5:
In file included from ./page/DOMTimer.cpp:32:
In file included from /Users/fred/WebKit/Source/WebCore/platform/Logging.h:28:
PAL/pal/LogMacros.h:36:9: note: expanded from macro 'LOG_WITH_STREAM'
        commands; \
        ^~~~~~~~
In file included from /Users/fred/WebKit/WebKitBuild/Debug-iphonesimulator/DerivedSources/WebCore/unified-sources/UnifiedSource315.cpp:5:
./page/DOMTimer.cpp:395:68: error: invalid operands to binary expression
      ('const char *' and 'int')
  ...LOG_WITH_STREAM(ContentObservation, "DOMTimer::fired(" << m_timeoutId <...
                                         ~~~~~~~~~~~~~~~~~~ ^  ~~~~~~~~~~~
In file included from /Users/fred/WebKit/WebKitBuild/Debug-iphonesimulator/DerivedSources/WebCore/unified-sources/UnifiedSource315.cpp:5:
In file included from ./page/DOMTimer.cpp:32:
In file included from /Users/fred/WebKit/Source/WebCore/platform/Logging.h:28:
PAL/pal/LogMacros.h:36:9: note: expanded from macro 'LOG_WITH_STREAM'
        commands; \
        ^~~~~~~~
3 errors generated.

** BUILD FAILED **

[zalan]

(In reply to Frédéric Wang (:fredw) from comment #8)
> @Zalan: This is making iOS debug build fails:
> https://build.webkit.org/builders/
> Apple%20iOS%2012%20Simulator%20Debug%20%28Build%29
> 
> n file included from
> /Users/fred/WebKit/WebKitBuild/Debug-iphonesimulator/DerivedSources/WebCore/
> unified-sources/UnifiedSource315.cpp:5:
> ./page/DOMTimer.cpp:242:102: error: invalid use of member 'm_timeoutId' in
>       static member function
>   ...<< "DOMTimer::install: register this timer: (" << m_timeoutId << ") and
> ...
>                                                        ^~~~~~~~~~~
> In file included from
> /Users/fred/WebKit/WebKitBuild/Debug-iphonesimulator/DerivedSources/WebCore/
> unified-sources/UnifiedSource315.cpp:5:
> In file included from ./page/DOMTimer.cpp:32:
> In file included from
> /Users/fred/WebKit/Source/WebCore/platform/Logging.h:28:
> PAL/pal/LogMacros.h:36:9: note: expanded from macro 'LOG_WITH_STREAM'
>         commands; \
>         ^~~~~~~~
> In file included from
> /Users/fred/WebKit/WebKitBuild/Debug-iphonesimulator/DerivedSources/WebCore/
> unified-sources/UnifiedSource315.cpp:5:
> ./page/DOMTimer.cpp:391:68: error: invalid operands to binary expression
>       ('const char *' and 'int')
>   ...LOG_WITH_STREAM(ContentObservation, "DOMTimer::fired(" << m_timeoutId
> <...
>                                          ~~~~~~~~~~~~~~~~~~ ^  ~~~~~~~~~~~
> In file included from
> /Users/fred/WebKit/WebKitBuild/Debug-iphonesimulator/DerivedSources/WebCore/
> unified-sources/UnifiedSource315.cpp:5:
> In file included from ./page/DOMTimer.cpp:32:
> In file included from
> /Users/fred/WebKit/Source/WebCore/platform/Logging.h:28:
> PAL/pal/LogMacros.h:36:9: note: expanded from macro 'LOG_WITH_STREAM'
>         commands; \
>         ^~~~~~~~
> In file included from
> /Users/fred/WebKit/WebKitBuild/Debug-iphonesimulator/DerivedSources/WebCore/
> unified-sources/UnifiedSource315.cpp:5:
> ./page/DOMTimer.cpp:395:68: error: invalid operands to binary expression
>       ('const char *' and 'int')
>   ...LOG_WITH_STREAM(ContentObservation, "DOMTimer::fired(" << m_timeoutId
> <...
>                                          ~~~~~~~~~~~~~~~~~~ ^  ~~~~~~~~~~~
> In file included from
> /Users/fred/WebKit/WebKitBuild/Debug-iphonesimulator/DerivedSources/WebCore/
> unified-sources/UnifiedSource315.cpp:5:
> In file included from ./page/DOMTimer.cpp:32:
> In file included from
> /Users/fred/WebKit/Source/WebCore/platform/Logging.h:28:
> PAL/pal/LogMacros.h:36:9: note: expanded from macro 'LOG_WITH_STREAM'
>         commands; \
>         ^~~~~~~~
> 3 errors generated.
> 
> ** BUILD FAILED **

:( Thanks! Fixed in r242039

[Frédéric Wang (:fredw)]

(In reply to zalan from comment #9)
> :( Thanks! Fixed in r242039

Sorry, I was not very explicit but there are 3 places to fix... buildbot still complains about the 2 remaining places: https://build.webkit.org/builders/Apple%20iOS%2012%20Simulator%20Debug%20%28Build%29/builds/3761/steps/compile-webkit/logs/stdio

[Frédéric Wang (:fredw)]

(In reply to Frédéric Wang (:fredw) from comment #10)
> (In reply to zalan from comment #9)
> > :( Thanks! Fixed in r242039
> 
> Sorry, I was not very explicit but there are 3 places to fix... buildbot
> still complains about the 2 remaining places:
> https://build.webkit.org/builders/
> Apple%20iOS%2012%20Simulator%20Debug%20%28Build%29/builds/3761/steps/compile-
> webkit/logs/stdio

Nevermind, I saw you just landed one more commit.

[zalan]

(In reply to Frédéric Wang (:fredw) from comment #10)
> (In reply to zalan from comment #9)
> > :( Thanks! Fixed in r242039
> 
> Sorry, I was not very explicit but there are 3 places to fix... buildbot
> still complains about the 2 remaining places:
> https://build.webkit.org/builders/
> Apple%20iOS%2012%20Simulator%20Debug%20%28Build%29/builds/3761/steps/compile-
> webkit/logs/stdio
Yeah, I noticed that too. https://trac.webkit.org/changeset/242041/webkit should take care of them all.