Bug 132731 - Web Inspector: Restore regionLayoutUpdated event in iOS7 inspector and remove regionOversetChanged
Summary: Web Inspector: Restore regionLayoutUpdated event in iOS7 inspector and remove...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Radu Stavila
URL:
Keywords: AdobeTracked, InRadar
Depends on:
Blocks: 57312
  Show dependency tree / graph
 
Reported: 2014-05-09 01:40 PDT by Radu Stavila
Modified: 2014-05-13 06:12 PDT (History)
5 users (show)

See Also:

Attachments
Patch (6.75 KB, patch)
2014-05-09 04:06 PDT, Radu Stavila 		timothy: review-
Details | Formatted Diff | Diff
Patch integrating reviewer feedback (5.27 KB, patch)
2014-05-12 02:27 PDT, Radu Stavila 		no flags Details | Formatted Diff | Diff
Patch integrating reviewer feedback (5.26 KB, patch)
2014-05-12 02:28 PDT, Radu Stavila 		joepeck: review+
Details | Formatted Diff | Diff
Patch for landing (5.26 KB, patch)
2014-05-13 04:04 PDT, Radu Stavila 		no flags Details | Formatted Diff | Diff
Retrying (5.26 KB, patch)
2014-05-13 04:51 PDT, Radu Stavila 		no flags Details | Formatted Diff | Diff
Show Obsolete (3) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Radu Stavila 2014-05-09 01:40:51 PDT 
Web Inspector: Restore regionLayoutUpdated event in iOS7 inspector and remove regionOversetChanged
Comment 1 Radar WebKit Bug Importer 2014-05-09 01:42:56 PDT 
<rdar://problem/16864929>
Comment 2 Radu Stavila 2014-05-09 04:06:15 PDT 
Created attachment 231140 [details]
Patch
Comment 3 Timothy Hatcher 2014-05-09 09:20:47 PDT 
Comment on attachment 231140 [details]
Patch

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

Looking good. We can reuse the new path internally. The observer change and event name map are really the only changes needed.

> Source/WebInspectorUI/UserInterface/Controllers/DOMTreeManager.js:61
> +    RegionLayoutUpdated: "dom-tree-manager-region-layout-updated",  /* COMPATIBILITY (iOS 7 and Safari 7): regionLayoutUpdated was removed and replaced by regionOversetChanged */

Only iOS has compatibility concerns. Safari on OS X always has a matching WebKit.

We can reuse the new event internally. The only compatibility layer needed is in the Observer file.

> Source/WebInspectorUI/UserInterface/Controllers/DOMTreeManager.js:620
> +    /* COMPATIBILITY (iOS 7 and Safari 7): regionLayoutUpdated was removed and replaced by regionOversetChanged */

Ditto.

> Source/WebInspectorUI/UserInterface/Models/ScriptTimelineRecord.js:193
> +        nameMap.set("webkitregionlayoutupdate", "Region Layout Update");    /* COMPATIBILITY (iOS 7 and Safari 7): regionLayoutUpdated was removed and replaced by regionOversetChanged */

This is needed.

> Source/WebInspectorUI/UserInterface/Protocol/CSSObserver.js:67
> +    regionLayoutUpdated: function(namedFlow)

And this is needed. But it can just regionOversetChanged.
Comment 4 Timothy Hatcher 2014-05-09 14:03:00 PDT 
Comment on attachment 231140 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Protocol/CSSObserver.js:66
> +    /* COMPATIBILITY (iOS 7 and Safari 7): regionLayoutUpdated was removed and replaced by regionOversetChanged */

Should just be: // COMPATIBILITY (iOS 7): ...
Comment 5 Radu Stavila 2014-05-12 02:27:27 PDT 
Created attachment 231280 [details]
Patch integrating reviewer feedback
Comment 6 Radu Stavila 2014-05-12 02:28:53 PDT 
Created attachment 231281 [details]
Patch integrating reviewer feedback
Comment 7 Radu Stavila 2014-05-12 02:31:59 PDT 
Comment on attachment 231281 [details]
Patch integrating reviewer feedback

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

> Source/WebInspectorUI/UserInterface/Protocol/Legacy/7.0/InspectorWebBackendCommands.js:206
> +InspectorBackend.registerEvent("CSS.regionLayoutUpdated", ["namedFlow"]);

I also left these 7.0 legacy rollbacks in because I made these changes in https://bugs.webkit.org/show_bug.cgi?id=132564 and https://bugs.webkit.org/show_bug.cgi?id=132566 and I now realise they were wrong. Please let me know if it's ok.

> Source/WebInspectorUI/Versions/Inspector-iOS-7.0.json:-2188
> -                "name": "regionOversetChanged",

Ditto.
Comment 8 Joseph Pecoraro 2014-05-12 10:16:42 PDT 
Comment on attachment 231281 [details]
Patch integrating reviewer feedback

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

r=me

> Source/WebInspectorUI/UserInterface/Models/ScriptTimelineRecord.js:193
> +        nameMap.set("webkitregionlayoutupdate", "Region Layout Update");    /* COMPATIBILITY (iOS 7): regionLayoutUpdated was removed and replaced by regionOversetChanged */

Style: This should be a single line comment, and should end in a period. We try not to use multiline comments.

> Source/WebInspectorUI/UserInterface/Protocol/CSSObserver.js:66
> +    /* COMPATIBILITY (iOS 7): regionLayoutUpdated was removed and replaced by regionOversetChanged */

Style: This should be a single line comment, and should end in a period. We try not to use multiline comments.
Comment 9 Radu Stavila 2014-05-13 04:04:56 PDT 
Created attachment 231367 [details]
Patch for landing
Comment 10 Radu Stavila 2014-05-13 04:51:15 PDT 
Created attachment 231369 [details]
Retrying
Comment 11 WebKit Commit Bot 2014-05-13 06:11:55 PDT 
Comment on attachment 231369 [details]
Retrying

Clearing flags on attachment: 231369

Committed r168678: <http://trac.webkit.org/changeset/168678>