Bug 194977

Summary: Introduce ContentChangeObserver class
Product: WebKit Reporter: zalan <zalan>
Component: Layout and RenderingAssignee: zalan <zalan>
Status: RESOLVED FIXED    
Severity: Normal CC: bfulgham, commit-queue, fred.wang, simon.fraser, webkit-bug-importer, zalan
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch none

Description zalan 2019-02-23 08:26:50 PST 
This class will eventually take care of all hover related content change observation logic.
Comment 1 Radar WebKit Bug Importer 2019-02-23 08:41:29 PST 
<rdar://problem/48338115>
Comment 2 zalan 2019-02-23 11:40:27 PST 
Created attachment 362829 [details]
Patch
Comment 3 Simon Fraser (smfr) 2019-02-23 19:35:44 PST 
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.
Comment 4 zalan 2019-02-24 21:02:33 PST 
Created attachment 362882 [details]
Patch
Comment 5 zalan 2019-02-24 21:13:42 PST 
(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()
Comment 6 WebKit Commit Bot 2019-02-24 22:04:26 PST 
Comment on attachment 362882 [details]
Patch

Clearing flags on attachment: 362882

Committed r242032: <https://trac.webkit.org/changeset/242032>
Comment 7 WebKit Commit Bot 2019-02-24 22:04:28 PST 
All reviewed patches have been landed.  Closing bug.
Comment 8 Frédéric Wang (:fredw) 2019-02-25 04:51:04 PST 
@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 **
Comment 9 zalan 2019-02-25 07:18:27 PST 
(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
Comment 10 Frédéric Wang (:fredw) 2019-02-25 07:51:23 PST 
(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
Comment 11 Frédéric Wang (:fredw) 2019-02-25 07:52:52 PST 
(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.
Comment 12 zalan 2019-02-25 07:53:02 PST 
(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.