WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
135073
Back/Forward arrows (modern) are too large.
https://bugs.webkit.org/show_bug.cgi?id=135073
Summary
Back/Forward arrows (modern) are too large.
Jonathan Wells
Reported
2014-07-18 15:33:42 PDT
These look heavy for their intended look. <
rdar://problem/17731035
>
Attachments
[PATCH] Attempted fix.
(6.58 KB, patch)
2014-07-18 16:15 PDT
,
Jonathan Wells
no flags
Details
Formatted Diff
Diff
[PATCH] Attempted fix.
(15.33 KB, patch)
2014-07-18 17:34 PDT
,
Jonathan Wells
joepeck
: review+
Details
Formatted Diff
Diff
[PATCH] Attempted fix, review feedback.
(15.77 KB, patch)
2014-07-21 11:26 PDT
,
Jonathan Wells
jonowells
: commit-queue-
Details
Formatted Diff
Diff
[PATCH] Attempted fix, review feedback + 1.
(15.80 KB, patch)
2014-07-21 11:44 PDT
,
Jonathan Wells
no flags
Details
Formatted Diff
Diff
Show Obsolete
(3)
View All
Add attachment
proposed patch, testcase, etc.
Jonathan Wells
Comment 1
2014-07-18 16:15:35 PDT
Created
attachment 235156
[details]
[PATCH] Attempted fix.
Joseph Pecoraro
Comment 2
2014-07-18 16:22:21 PDT
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
Comment 3
2014-07-18 17:26:32 PDT
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
Comment 4
2014-07-18 17:34:36 PDT
Created
attachment 235160
[details]
[PATCH] Attempted fix.
Timothy Hatcher
Comment 5
2014-07-18 18:09:42 PDT
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 6
2014-07-18 18:10:26 PDT
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
Comment 7
2014-07-21 11:26:10 PDT
Created
attachment 235231
[details]
[PATCH] Attempted fix, review feedback.
Joseph Pecoraro
Comment 8
2014-07-21 11:39:42 PDT
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
Comment 9
2014-07-21 11:44:39 PDT
Created
attachment 235232
[details]
[PATCH] Attempted fix, review feedback + 1.
WebKit Commit Bot
Comment 10
2014-07-21 12:16:50 PDT
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
Comment 11
2014-07-21 12:16:53 PDT
All reviewed patches have been landed. Closing bug.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug