Bug 132564

Summary: [CSS Regions] Remove regionLayoutUpdate event
Product: WebKit Reporter: Radu Stavila <stavila>
Component: CSSAssignee: Radu Stavila <stavila>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, esprehn+autocc, glenn, graouts, joepeck, kangil.han, kondapallykalyan, syoichi, timothy, WebkitBugTracker
Priority: P2 Keywords: AdobeTracked
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 57312    
Attachments:
Description Flags
Patch none

Description Radu Stavila 2014-05-05 06:45:46 PDT
The regionLayoutUpdate event is deprecated and should be removed. It was replaced by the regionOversetChange event (http://blogs.adobe.com/webplatform/2013/07/15/css-regions-new-working-draft/)
Comment 1 Radu Stavila 2014-05-05 09:10:54 PDT
Created attachment 230832 [details]
Patch
Comment 2 Radu Stavila 2014-05-05 09:11:26 PDT
Latest CSS Regions WD: http://www.w3.org/TR/css3-regions/
Comment 3 WebKit Commit Bot 2014-05-05 11:50:23 PDT
Comment on attachment 230832 [details]
Patch

Clearing flags on attachment: 230832

Committed r168306: <http://trac.webkit.org/changeset/168306>
Comment 4 WebKit Commit Bot 2014-05-05 11:50:28 PDT
All reviewed patches have been landed.  Closing bug.
Comment 5 Joseph Pecoraro 2014-05-08 10:32:40 PDT
Comment on attachment 230832 [details]
Patch

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

To clarify my comments, I only object to a few of the WebInspectorUI changes. The frontend is meant to be backwards compatible with older backends (e.g. iOS 6 and iOS 7). Where possible we should support what the older backends were legitimately capable of doing. Unless something complex changes that would make supporting a legacy case difficult (which I don't believe is the case here), we should strive to maintain backwards compatibility.

Eventually if we decide to stop supporting inspecting iOS 7 we can find the COMPATIBILITY comments and remove the associated code.

> Source/WebInspectorUI/UserInterface/Controllers/DOMTreeManager.js:-61
> -    RegionLayoutUpdated: "dom-tree-manager-region-layout-updated",

So this wasn't used?

> Source/WebInspectorUI/UserInterface/Controllers/DOMTreeManager.js:-623
> -    regionLayoutUpdated: function(flowPayload)
> -    {
> -        this._sendNamedFlowUpdateEvents(flowPayload);
> -    },

These should be left in for legacy.

> Source/WebInspectorUI/UserInterface/Models/ScriptTimelineRecord.js:-193
> -        nameMap.set("webkitregionlayoutupdate", "Region Layout Update");

You could put a COMPATIBILITY comment here.

> Source/WebInspectorUI/UserInterface/Protocol/CSSObserver.js:-69
> -    regionLayoutUpdated: function(namedFlow)
> -    {
> -        WebInspector.domTreeManager.regionLayoutUpdated(namedFlow);
> -    },

Here in the Observer we should add a comment that this is for legacy only. Something like:

    // COMPATIBILITY (iOS 7): regionLayoutUpdated has transitioned to regionOversetChanged.

> Source/WebInspectorUI/Versions/Inspector-iOS-7.0.json:-2192
> -            },
> -            {
> -                "name": "regionLayoutUpdated",
> -                "parameters": [
> -                    { "name": "namedFlow", "$ref": "NamedFlow", "description": "The Named Flow whose layout may have changed." }
> -                ],
> -                "description": "Fires when a Named Flow's layout may have changed."

So technically this still exists in iOS 7 and we should leave it in and handle it for the best debugging experience.
Comment 6 Radu Stavila 2014-05-09 01:44:25 PDT
Created https://bugs.webkit.org/show_bug.cgi?id=132731