Bug 93443 – Web Inspector: Protocol Extension: Add "regionLayoutUpdate" event

• Component: Web Inspector (Deprecated)
• Status: RESOLVED FIXED

[Andrei Poenaru]

Extend the protocol by adding support for "regionLayoutUpdate" event.
http://www.w3.org/TR/css3-regions/#region-flow-layout-events

[Andrei Poenaru]

Created attachment 158311 [details]
Patch

[Pavel Feldman]

Comment on attachment 158311 [details]
Patch

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

> Source/WebCore/inspector/InspectorCSSAgent.cpp:838
> +    if (!m_namedFlowCollectionsRequested.contains(documentNodeId)) {

I think m_namedFlowCollectionsRequested is superflous now that you have the more complete m_documentIdToNamedFlowSetMap.

> Source/WebCore/inspector/InspectorCSSAgent.cpp:868
> +            it = m_documentIdToNamedFlowSetMap.add(documentNodeId, NamedFlowSet()).iterator;

You should register this flow in the m_documentIdToNamedFlowSetMap from getNamedFlowCollection as well. I think it is better to do that in buildObjectForNamedFlow. You collection tracks elements that are sent to the front-end and the buildObjectForNamedFlow is a perfect place to do that.

> Source/WebCore/inspector/InspectorCSSAgent.cpp:1162
> +    int documentNodeId = m_domAgent->boundNodeId(document);

Looks like too much copypaste (add the "remove" parameter to the original method? reuse same implementation?)

[Andrei Poenaru]

(In reply to comment #2)
> (From update of attachment 158311 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=158311&action=review
> 
> > Source/WebCore/inspector/InspectorCSSAgent.cpp:838
> > +    if (!m_namedFlowCollectionsRequested.contains(documentNodeId)) {
> 
> I think m_namedFlowCollectionsRequested is superflous now that you have the more complete m_documentIdToNamedFlowSetMap.

I need a way to verify if the Named Flow Collection has been requested for a given document (to see if the NamedFlowCreated event should be dispatched).
A solution would be to add a NULL pointer to the NamedFlowSet when the Named Flow Collection for some document is sent to the front-end: this  will lead to a NamedFlowSet containing a single element.

[Pavel Feldman]

> I need a way to verify if the Named Flow Collection has been requested for a given document (to see if the NamedFlowCreated event should be dispatched).
> A solution would be to add a NULL pointer to the NamedFlowSet when the Named Flow Collection for some document is sent to the front-end: this  will lead to a NamedFlowSet containing a single element.

Not sure I follow. You get a request: getNamedFlowCollection, you return collection of flows for given document and populate m_documentIdToNamedFlowSetMap. If document had no flows, m_documentIdToNamedFlowSetMap would contain empty set for that document id. What do I miss?

[Andrei Poenaru]

(In reply to comment #4)

> Not sure I follow. You get a request: getNamedFlowCollection, you return collection of flows for given document and populate m_documentIdToNamedFlowSetMap. If document had no flows, m_documentIdToNamedFlowSetMap would contain empty set for that document id. What do I miss?

There are 2 situations:

1) getNamedFlowCollection is called for a given document: you dispatch all events related to all named flows from that document.

2) getFlowByName is called without calling getNamedFlowCollection: you only dispatch RegionLayoutUpdate and NamedFlowRemoved for the requested named flow.

If on getNamedFlowCollection the NamedFlowSet is just filled with named flows you will not be able to tell if those flows were requested separately or if the entire collection has been requested.
This information is used in order to decide if the front-end should be informed about a flow creation (the NamedFlowCreated event).

[Pavel Feldman]

Ok, I understand it now. I was under the assumption that prior to calling getFlowByName, front-end would need to call getNamedFlowCollections in order to discover the names. So these two requests were overlapping for me. So there will be a UI where user would type in the flow name?

I would suggest that you only leave getNamedFlowCollections. That would make front-end listen to created flows, but that is alright. As you mentioned, the traffic and cost of building the payloads is not high for these objects. Leaving only that method would simplify the backend code, reduce the API surface and reduce the overlap. What do you think?

[Andrei Poenaru]

(In reply to comment #6)
> So there will be a UI where user would type in the flow name?

I was thinking about dubroy's mockup:
https://bug-90789-attachments.webkit.org/attachment.cgi?id=152749

> I would suggest that you only leave getNamedFlowCollections. That would make front-end listen to created flows, but that is alright. As you mentioned, the traffic and cost of building the payloads is not high for these objects. Leaving only that method would simplify the backend code, reduce the API surface and reduce the overlap. What do you think?

It should be ok to remove the getFlowByName function. The front-end could keep a requested named flow collection in sync with the backend. In this case, the NamedFlowCreated and RegionLayoutUpdate events should send named flows, while NamedFlowRemoved should only send the name of the flow.

[Pavel Feldman]

> I was thinking about dubroy's mockup:
> https://bug-90789-attachments.webkit.org/attachment.cgi?id=152749
> 

I don't think sidebar is a good place for extensibility, you now have a drawer that has more real estate where user can focus on the flows.

> It should be ok to remove the getFlowByName function. The front-end could keep a requested named flow collection in sync with the backend. In this case, the NamedFlowCreated and RegionLayoutUpdate events should send named flows, while NamedFlowRemoved should only send the name of the flow.

I would proceed this way.

[Andrei Poenaru]

Created attachment 160934 [details]
Patch

[Pavel Feldman]

Comment on attachment 160934 [details]
Patch

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

Overall looks good. A number of renaming suggestions inline.

> Source/WebCore/dom/NamedFlowCollection.cpp:98
> +    InspectorInstrumentation::didRemoveNamedFlow(m_document, namedFlow);

willRemoveNamedFlow? (it is now happening before the actual removal)

> Source/WebCore/inspector/Inspector.json:2446
> +                "name": "regionLayoutUpdate",

regionLayoutUpdated?

> Source/WebCore/inspector/InspectorCSSAgent.cpp:542
> +    int documentNodeId = m_domAgent->boundNodeId(document);

Sounds like this is checked 3 times. A helper method documentNodeWithRequestedFlowsId?

> Source/WebCore/inspector/front-end/CSSStyleModel.js:381
> +        if (!this.hasEventListeners(WebInspector.CSSStyleModel.Events.NamedFlowCreated))

It is too late for this check, it does not save anything.

> Source/WebCore/inspector/front-end/CSSStyleModel.js:398
>          if (!this.hasEventListeners(WebInspector.CSSStyleModel.Events.NamedFlowRemoved))

ditto

> Source/WebCore/inspector/front-end/CSSStyleModel.js:415
> +        if (!this.hasEventListeners(WebInspector.CSSStyleModel.Events.RegionLayoutUpdate))

ditto

> Source/WebCore/inspector/front-end/CSSStyleModel.js:488
> +        this._namedFlowCollections = {};

Should this fire an event?

> Source/WebCore/inspector/front-end/CSSStyleModel.js:1436
> +    getFlowByName: function(flowName)

flowByName

[Andrei Poenaru]

(In reply to comment #10)
> > Source/WebCore/inspector/front-end/CSSStyleModel.js:488
> > +        this._namedFlowCollections = {};
> 
> Should this fire an event?

I thought of it as a method to reset the cache when the document is updated, so I do not think it should fire an event.

[Andrei Poenaru]

Created attachment 161884 [details]
Patch

[Pavel Feldman]

Comment on attachment 161884 [details]
Patch

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

> Source/WebCore/rendering/RenderNamedFlowThread.cpp:340
> +    if (!m_regionLayoutUpdateEventTimer.isActive())

Does not look like a part of this change. Web Inspector part lgtm, please make sure this one is fine with the code owners prior to landing.

[Andrei Poenaru]

Created attachment 161931 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 161931 [details]
Patch for landing

Rejecting attachment 161931 [details] from commit-queue.

New failing tests:
http/tests/cache/subresource-expiration-1.html
http/tests/cache/stopped-revalidation.html
Full output: http://queues.webkit.org/results/13732882

[WebKit Review Bot]

Comment on attachment 161931 [details]
Patch for landing

Rejecting attachment 161931 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
pyftpdlib/src --revision 977 --non-interactive --force --accept theirs-conflict --ignore-externals returned non-zero exit status 1 in /mnt/git/webkit-commit-queue/Source/WebKit/chromium
Error: 'depot_tools/gclient sync --force --reset --delete_unversioned_trees' failed 3 tries and returned 256 at Tools/Scripts/update-webkit-chromium line 96.
Re-trying 'depot_tools/gclient sync --force --reset --delete_unversioned_trees'
Died at /mnt/git/webkit-commit-queue/Tools/Scripts/webkitdirs.pm line 2525.

Full output: http://queues.webkit.org/results/13746440

[Andrei Poenaru]

Created attachment 162029 [details]
Rebased Patch

[Vsevolod Vlasov]

Committed r127468: <http://trac.webkit.org/changeset/127468>

[Jessie Berlin]

(In reply to comment #18)
> Committed r127468: <http://trac.webkit.org/changeset/127468>

This caused assertion failures - "crashes" on the ML Debug WK1 Tests:

http://build.webkit.org/builders/Apple%20MountainLion%20Debug%20WK1%20%28Tests%29/builds/492
http://build.webkit.org/results/Apple%20MountainLion%20Debug%20WK1%20(Tests)/r127468%20(493)/inspector/styles/protocol-css-regions-commands-crash-log.txt

Please fix this soon or I will have to roll out this patch.

Exception Type:  EXC_BAD_ACCESS (SIGSEGV)
Exception Codes: KERN_INVALID_ADDRESS at 0x00000000bbadbeef

VM Regions Near 0xbbadbeef:
--> 
    __TEXT                 000000010a816000-000000010a8b7000 [  644K] r-x/rwx SM=COW  /Volumes/VOLUME/*

Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   com.apple.WebCore             	0x000000010c3216eb WebCore::Document::updateStyleIfNeeded() + 219 (Document.cpp:1901)
1   com.apple.WebCore             	0x000000010c32192b WebCore::Document::updateLayout() + 155 (Document.cpp:1933)
2   com.apple.WebCore             	0x000000010c321a82 WebCore::Document::updateLayoutIgnorePendingStylesheets() + 210 (Document.cpp:1969)
3   com.apple.WebCore             	0x000000010d7c2dc6 WebCore::WebKitNamedFlow::getContent() + 118 (WebKitNamedFlow.cpp:164)
4   com.apple.WebCore             	0x000000010c8c38e4 WebCore::InspectorCSSAgent::buildObjectForNamedFlow(WTF::String*, WebCore::WebKitNamedFlow*, int) + 68 (InspectorCSSAgent.cpp:1111)
5   com.apple.WebCore             	0x000000010c8c3c25 WebCore::InspectorCSSAgent::didUpdateRegionLayout(WebCore::Document*, WebCore::WebKitNamedFlow*) + 117 (InspectorCSSAgent.cpp:566)
6   com.apple.WebCore             	0x000000010c928720 WebCore::InspectorInstrumentation::didUpdateRegionLayoutImpl(WebCore::InstrumentingAgents*, WebCore::Document*, WebCore::WebKitNamedFlow*) + 64 (InspectorInstrumentation.cpp:250)
7   com.apple.WebCore             	0x000000010d233d5e WebCore::InspectorInstrumentation::didUpdateRegionLayout(WebCore::Document*, WebCore::WebKitNamedFlow*) + 78 (InspectorInstrumentation.h:591)
8   com.apple.WebCore             	0x000000010d23301b WebCore::RenderNamedFlowThread::dispatchRegionLayoutUpdateEvent() + 75 (RenderNamedFlowThread.cpp:342)
9   com.apple.WebCore             	0x000000010d1a2c25 WebCore::RenderFlowThread::layout() + 1429 (RenderFlowThread.cpp:196)
10  com.apple.WebCore             	0x000000010c5b3326 WebCore::RenderObject::layoutIfNeeded() + 54 (RenderObject.h:655)
11  com.apple.WebCore             	0x000000010c5b28a6 WebCore::FlowThreadController::layoutRenderNamedFlowThreads() + 1126 (FlowThreadController.cpp:134)
12  com.apple.WebCore             	0x000000010d354aaa WebCore::RenderView::layout() + 1066 (RenderView.cpp:159)
13  com.apple.WebCore             	0x000000010c63f2db WebCore::FrameView::layout(bool) + 3147 (FrameView.cpp:1182)
14  com.apple.WebCore             	0x000000010c63b1a0 WebCore::FrameView::layoutTimerFired(WebCore::Timer<WebCore::FrameView>*) + 32 (FrameView.cpp:2105)
15  com.apple.WebCore             	0x000000010c6563b3 WebCore::Timer<WebCore::FrameView>::fired() + 115 (Timer.h:100)
16  com.apple.WebCore             	0x000000010d71f37d WebCore::ThreadTimers::sharedTimerFiredInternal() + 285 (ThreadTimers.cpp:118)
17  com.apple.WebCore             	0x000000010d71f119 WebCore::ThreadTimers::sharedTimerFired() + 25 (ThreadTimers.cpp:94)
18  com.apple.WebCore             	0x000000010d460553 WebCore::timerFired(__CFRunLoopTimer*, void*) + 67 (SharedTimerMac.mm:167)

(In reply to comment #19)
> (In reply to comment #18)
> > Committed r127468: <http://trac.webkit.org/changeset/127468>
> 
> This caused assertion failures - "crashes" on the ML Debug WK1 Tests:
> 
> http://build.webkit.org/builders/Apple%20MountainLion%20Debug%20WK1%20%28Tests%29/builds/492
> http://build.webkit.org/results/Apple%20MountainLion%20Debug%20WK1%20(Tests)/r127468%20(493)/inspector/styles/protocol-css-regions-commands-crash-log.txt

I reverted r127468 in <http://trac.webkit.org/r127500>.

[Andrei Poenaru]

Created attachment 162236 [details]
Patch

[Alexander Pavlov (apavlov)]

Comment on attachment 162236 [details]
Patch

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

> Source/WebCore/inspector/front-end/CSSStyleModel.js:1403
> +    for (var i = 0; i < payload.length; ++i) {

This use without any preliminary checks is going to fail, given that you declare payload as nullable and optional (any reason for it to be both?) From the code, it seems that payload can be neither null, nor undefined (unless null comes in from the backend).

> Source/WebCore/inspector/front-end/CSSStyleModel.js:1434
> +        if (namedFlow === undefined)

According to the WebKit coding guidelines, this should be "if (!namedFlow)"

[Andrei Poenaru]

Created attachment 162247 [details]
Patch

[WebKit Review Bot]

Comment on attachment 162247 [details]
Patch

Clearing flags on attachment: 162247

Committed r127700: <http://trac.webkit.org/changeset/127700>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

Re-opened since this is blocked by 95941

[Vsevolod Vlasov]

Compiling "sdk"
Source/WebCore/inspector/front-end/CSSStyleModel.js:226: ERROR - variable error is undeclared
            if (error || !namedFlowMap)
                ^

1 error(s), 0 warning(s)

[Vsevolod Vlasov]

Please use Source/WebCore/inspector/compile-front-end.py script to check for compilation errors before landing the patch.

Inspector currently uses closure compiler revision 1810: http://mvnrepository.com/artifact/com.google.javascript/closure-compiler/r1810

[Vsevolod Vlasov]

Reverted: Committed r127703: <http://trac.webkit.org/changeset/127703>

[Andrei Poenaru]

Created attachment 162476 [details]
Patch

[Andrei Poenaru]

Created attachment 162526 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 162526 [details]
Patch for landing

Clearing flags on attachment: 162526

Committed r127780: <http://trac.webkit.org/changeset/127780>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Vsevolod Vlasov]

FYI this patch broke closure compilation again, I have landed a fix. http://trac.webkit.org/changeset/127859

[WebKit Review Bot]

Re-opened since this is blocked by 96102

[Andrei Poenaru]

Created attachment 163066 [details]
Patch

This patch does not add any errors or warnings to the output of the compile-front-end.py script. 
Used this closure compiler: http://code.google.com/p/closure-compiler/

[Vsevolod Vlasov]

Does it fix test timeouts?

[Andrei Poenaru]

Created attachment 163559 [details]
Patch

[Alexander Pavlov (apavlov)]

Comment on attachment 163559 [details]
Patch

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

The change looks good, but I have a CSSStyleModel API concern now that getNamedFlowCollectionAsync() is made use of.

> Source/WebCore/inspector/InspectorCSSAgent.cpp:274
> +        namedFlows.append(std::make_pair(it->first, it->second));

You need to double-check, but seemingly, namedFlows.append(*it); will have the same effect.

> Source/WebCore/inspector/front-end/CSSStyleModel.js:180
> +    getNamedFlowCollectionAsync: function(documentNodeId, userCallback)

The method is named getNamedFlowCollectionAsync, however its result is "namedFlowCollection.namedFlowMap". Looking at its sole usage (in the real code), I guess it would be more appropriate to return namedFlowCollection to the userCallback (e.g. you wouldn't have to "this._namedFlowCollections[documentNodeId].flowByName(flowName)" in getFlowByNameAsync but rather "namedFlowCollection.flowByName(flowName)") - otherwise the returned result is just used to check for errors and discarded afterwards.

[Andrei Poenaru]

Created attachment 163580 [details]
Patch

[Alexander Pavlov (apavlov)]

Comment on attachment 163580 [details]
Patch

r=me

[WebKit Review Bot]

Comment on attachment 163580 [details]
Patch

Clearing flags on attachment: 163580

Committed r128293: <http://trac.webkit.org/changeset/128293>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.