Bug 93443

Summary: Web Inspector: Protocol Extension: Add "regionLayoutUpdate" event
Product: WebKit Reporter: Andrei Poenaru <andreigpoenaru>
Component: Web Inspector (Deprecated)Assignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: apavlov, bweinstein, eric, jberlin, joepeck, keishi, loislo, mhahnenberg, mitz, pfeldman, pmuellr, rik, timothy, vsevik, webkit.review.bot, yurys
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on: 95941, 96102    
Bug Blocks: 90786, 90871    
Attachments:
Description Flags
Patch
pfeldman: review-, pfeldman: commit-queue-
Patch
pfeldman: review-
Patch
none
Patch for landing
webkit.review.bot: commit-queue-
Rebased Patch
none
Patch
none
Patch
none
Patch
none
Patch for landing
none
Patch
none
Patch
none
Patch none

Description Andrei Poenaru 2012-08-08 00:02:58 PDT
Extend the protocol by adding support for "regionLayoutUpdate" event.
http://www.w3.org/TR/css3-regions/#region-flow-layout-events
Comment 1 Andrei Poenaru 2012-08-14 06:15:42 PDT
Created attachment 158311 [details]
Patch
Comment 2 Pavel Feldman 2012-08-17 05:10:06 PDT
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?)
Comment 3 Andrei Poenaru 2012-08-21 03:46:01 PDT
(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.
Comment 4 Pavel Feldman 2012-08-21 08:36:44 PDT
> 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?
Comment 5 Andrei Poenaru 2012-08-21 10:20:05 PDT
(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).
Comment 6 Pavel Feldman 2012-08-21 11:54:13 PDT
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?
Comment 7 Andrei Poenaru 2012-08-21 14:01:38 PDT
(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.
Comment 8 Pavel Feldman 2012-08-21 14:07:37 PDT
> 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.
Comment 9 Andrei Poenaru 2012-08-28 02:19:28 PDT
Created attachment 160934 [details]
Patch
Comment 10 Pavel Feldman 2012-08-31 02:08:37 PDT
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
Comment 11 Andrei Poenaru 2012-09-03 01:04:17 PDT
(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.
Comment 12 Andrei Poenaru 2012-09-03 02:29:16 PDT
Created attachment 161884 [details]
Patch
Comment 13 Pavel Feldman 2012-09-03 06:21:11 PDT
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.
Comment 14 Andrei Poenaru 2012-09-03 08:16:57 PDT
Created attachment 161931 [details]
Patch for landing
Comment 15 WebKit Review Bot 2012-09-03 12:40:01 PDT
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
Comment 16 WebKit Review Bot 2012-09-04 05:40:24 PDT
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
Comment 17 Andrei Poenaru 2012-09-04 06:32:17 PDT
Created attachment 162029 [details]
Rebased Patch
Comment 18 Vsevolod Vlasov 2012-09-04 08:32:09 PDT
Committed r127468: <http://trac.webkit.org/changeset/127468>
Comment 19 Jessie Berlin 2012-09-04 10:27:09 PDT
(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)
Comment 21 Andrei Poenaru 2012-09-05 06:47:40 PDT
Created attachment 162236 [details]
Patch
Comment 22 Alexander Pavlov (apavlov) 2012-09-05 07:22:28 PDT
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)"
Comment 23 Andrei Poenaru 2012-09-05 07:57:12 PDT
Created attachment 162247 [details]
Patch
Comment 24 WebKit Review Bot 2012-09-05 23:48:28 PDT
Comment on attachment 162247 [details]
Patch

Clearing flags on attachment: 162247

Committed r127700: <http://trac.webkit.org/changeset/127700>
Comment 25 WebKit Review Bot 2012-09-05 23:48:33 PDT
All reviewed patches have been landed.  Closing bug.
Comment 26 WebKit Review Bot 2012-09-06 00:26:19 PDT
Re-opened since this is blocked by 95941
Comment 27 Vsevolod Vlasov 2012-09-06 00:27:00 PDT
Compiling "sdk"
Source/WebCore/inspector/front-end/CSSStyleModel.js:226: ERROR - variable error is undeclared
            if (error || !namedFlowMap)
                ^

1 error(s), 0 warning(s)
Comment 28 Vsevolod Vlasov 2012-09-06 00:34:47 PDT
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
Comment 29 Vsevolod Vlasov 2012-09-06 00:35:13 PDT
Reverted: Committed r127703: <http://trac.webkit.org/changeset/127703>
Comment 30 Andrei Poenaru 2012-09-06 04:46:50 PDT
Created attachment 162476 [details]
Patch
Comment 31 Andrei Poenaru 2012-09-06 09:48:14 PDT
Created attachment 162526 [details]
Patch for landing
Comment 32 WebKit Review Bot 2012-09-06 13:19:18 PDT
Comment on attachment 162526 [details]
Patch for landing

Clearing flags on attachment: 162526

Committed r127780: <http://trac.webkit.org/changeset/127780>
Comment 33 WebKit Review Bot 2012-09-06 13:19:24 PDT
All reviewed patches have been landed.  Closing bug.
Comment 34 Vsevolod Vlasov 2012-09-07 05:27:42 PDT
FYI this patch broke closure compilation again, I have landed a fix. http://trac.webkit.org/changeset/127859
Comment 35 WebKit Review Bot 2012-09-07 06:54:40 PDT
Re-opened since this is blocked by 96102
Comment 36 Andrei Poenaru 2012-09-10 01:41:50 PDT
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/
Comment 37 Vsevolod Vlasov 2012-09-10 02:10:32 PDT
Does it fix test timeouts?
Comment 38 Andrei Poenaru 2012-09-12 02:35:26 PDT
Created attachment 163559 [details]
Patch
Comment 39 Alexander Pavlov (apavlov) 2012-09-12 03:02:24 PDT
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.
Comment 40 Andrei Poenaru 2012-09-12 04:23:31 PDT
Created attachment 163580 [details]
Patch
Comment 41 Alexander Pavlov (apavlov) 2012-09-12 04:40:43 PDT
Comment on attachment 163580 [details]
Patch

r=me
Comment 42 WebKit Review Bot 2012-09-12 05:07:03 PDT
Comment on attachment 163580 [details]
Patch

Clearing flags on attachment: 163580

Committed r128293: <http://trac.webkit.org/changeset/128293>
Comment 43 WebKit Review Bot 2012-09-12 05:07:11 PDT
All reviewed patches have been landed.  Closing bug.