[webkit-reviews] review granted: [Bug 135073] Back/Forward arrows (modern) are too large. : [Attachment 235160] [PATCH] Attempted fix.

[bugzilla-daemon at webkit.org]

Joseph Pecoraro <joepeck at webkit.org> has granted Jonathan Wells
<jonowells at apple.com>'s request for review:
Bug 135073: Back/Forward arrows (modern) are too large.
https://bugs.webkit.org/show_bug.cgi?id=135073

Attachment 235160: [PATCH] Attempted fix.
https://bugs.webkit.org/attachment.cgi?id=235160&action=review

------- Additional Comments from Joseph Pecoraro <joepeck at webkit.org>
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.