Bug 135073 – Back/Forward arrows (modern) are too large.

• Component: Web Inspector
• Status: RESOLVED FIXED

[Jonathan Wells]

These look heavy for their intended look.

<rdar://problem/17731035>

[Jonathan Wells]

Created attachment 235156 [details]
[PATCH] Attempted fix.

[Joseph Pecoraro]

Comment on attachment 235156 [details]
[PATCH] Attempted fix.

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

This looks good, Tim may be better.

> Source/WebInspectorUI/UserInterface/Base/ImageUtilities.js:27
> +const _imageStorageFormatVersion = 2;

Maybe we can avoid bumping the global version and make generateColoredImagesForCSS do imageVersion magic like the other path.

[Jonathan Wells]

I think we can use that computed version number as a default for all ImageUtilities.js images unless one is specified in `specifications` and passed in as a parameter for a particular image.

[Jonathan Wells]

Created attachment 235160 [details]
[PATCH] Attempted fix.

[Timothy Hatcher]

Comment on attachment 235160 [details]
[PATCH] Attempted fix.

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

> Source/WebInspectorUI/UserInterface/Base/Main.js:192
> +    if (versionMatch && versionMatch[1].indexOf("+") !== -1 && document.styleSheets.length < 10) {

This seems like it should move to Platform.js too.

[Joseph Pecoraro]

Comment on attachment 235160 [details]
[PATCH] Attempted fix.

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

r=me. Have you tested on a Retina machine?

> Source/WebInspectorUI/UserInterface/Base/Platform.js:26
> +// Add global platform information.

Nit: Unnecessary comment. Maybe this could go in the block below though.

> Source/WebInspectorUI/UserInterface/Base/Platform.js:29
> +    codeName: "",

Nit: Discussed on IRC, making this version.name sounds better!

> Source/WebInspectorUI/UserInterface/Base/Platform.js:43
> +        toString: function()
> +        {
> +            return this.base + "." + this.version;
> +        }
> +    },
> +    toString: function()
> +    {
> +        return this.name;
> +    }

Nit: Unnecessary toStrings. The only people that will debug this are us, and we can just inspect the object or JSON.stringify it.

> Source/WebInspectorUI/UserInterface/Base/Platform.js:65
> +var isLegacyMacOS = false;
> +var osVersionMatch = / Mac OS X (\d+)_(\d+)/.exec(navigator.appVersion);
> +if (osVersionMatch && osVersionMatch[1] === "10") {
> +    WebInspector.Platform.version.base = 10;
> +    switch(osVersionMatch[2]) {
> +        case "10":
> +            WebInspector.Platform.codeName = "yosemite";
> +            WebInspector.Platform.version.release = 10;
> +            break;
> +        case "9":
> +            WebInspector.Platform.codeName = "mavericks";
> +            WebInspector.Platform.version.release = 9;
> +            WebInspector.Platform.isLegacyMacOS = true;
> +            break;
> +        case "8":
> +            WebInspector.Platform.codeName = "mountain-lion";
> +            WebInspector.Platform.version.release = 8;
> +            WebInspector.Platform.isLegacyMacOS = true;
> +    }
> +}

I cringe a bit to have these in the global namespace. I suggest we wrap this work in an anonymous block:

    (function() {
        ...
    })();

Like the Load* files.

[Jonathan Wells]

Created attachment 235231 [details]
[PATCH] Attempted fix, review feedback.

[Joseph Pecoraro]

Comment on attachment 235231 [details]
[PATCH] Attempted fix, review feedback.

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

> Source/WebInspectorUI/UserInterface/Base/Platform.js:60
> +                WebInspector.Platform.isLegacyMacOS = true;

Nit: Would be nice to put a break here even though it is the last.

[Jonathan Wells]

Created attachment 235232 [details]
[PATCH] Attempted fix, review feedback + 1.

[WebKit Commit Bot]

Comment on attachment 235232 [details]
[PATCH] Attempted fix, review feedback + 1.

Clearing flags on attachment: 235232

Committed r171312: <http://trac.webkit.org/changeset/171312>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.