Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2201443002:
Md/Vectorize cellular network iconography. (Closed)
|
Created:
4 years, 4 months ago by Evan Stade Modified:
4 years, 4 months ago CC:
chromium-reviews, stevenjb+watch_chromium.org, oshima+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMd/Vectorize cellular network iconography.
Side note: if we had just used .icon files, besides the massive proliferation of them that would have been necessary, we'd have worse rendering at fractional scales like 1.5x because we'd be filling fractional pixels. However, to actually get the better drawing at fractional scales, the network technology badges (LTE, G, etc.) will also have to be vectorized.
BUG=617306
Committed: https://crrev.com/b82f940c45a71ce43624cd915ea9a62c838ca90c
Cr-Commit-Position: refs/heads/master@{#408803}
Patch Set 1 #
Total comments: 5
Patch Set 2 : . #
Messages
Total messages: 21 (11 generated)
estade@chromium.org changed reviewers: + oshima@chromium.org, stevenjb@chromium.org, tdanderson@chromium.org
+tdanderson for review +stevenjb and/or oshima for review and OWNERS approval
The CQ bit was checked by estade@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
LGTM! https://codereview.chromium.org/2201443002/diff/1/ui/chromeos/network/network... File ui/chromeos/network/network_icon.cc (right): https://codereview.chromium.org/2201443002/diff/1/ui/chromeos/network/network... ui/chromeos/network/network_icon.cc:293: image_type_ == ARCS ? kNumArcsImages : kNumBarsImages); Maybe consolidate these two constants into kNumNetworkImages or similar? https://codereview.chromium.org/2201443002/diff/1/ui/chromeos/network/network... ui/chromeos/network/network_icon.cc:395: // TODO(estade): share this alpha with other things in ash (battery, etc.). Consider referencing crbug.com/623987
lgtm https://codereview.chromium.org/2201443002/diff/1/ui/chromeos/network/network... File ui/chromeos/network/network_icon.cc (right): https://codereview.chromium.org/2201443002/diff/1/ui/chromeos/network/network... ui/chromeos/network/network_icon.cc:293: image_type_ == ARCS ? kNumArcsImages : kNumBarsImages); On 2016/07/29 21:43:29, tdanderson wrote: > Maybe consolidate these two constants into kNumNetworkImages or similar? FWIW, the number of arcs/bars images used to differ. I am in favor of consolidating these.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Md/Vectorize cellular network iconography. BUG=617306 ========== to ========== Md/Vectorize cellular network iconography. Side note: if we had just used .icon files, besides the massive proliferation of them that would have been necessary, we'd have worse rendering at fractional scales like 1.5x because we'd be filling fractional pixels. However, to actually get the better drawing at fractional scales, the network technology badges (LTE, G, etc.) will also have to be vectorized. BUG=617306 ==========
https://codereview.chromium.org/2201443002/diff/1/ui/chromeos/network/network... File ui/chromeos/network/network_icon.cc (right): https://codereview.chromium.org/2201443002/diff/1/ui/chromeos/network/network... ui/chromeos/network/network_icon.cc:293: image_type_ == ARCS ? kNumArcsImages : kNumBarsImages); On 2016/07/29 21:47:05, stevenjb wrote: > On 2016/07/29 21:43:29, tdanderson wrote: > > Maybe consolidate these two constants into kNumNetworkImages or similar? > > FWIW, the number of arcs/bars images used to differ. I am in favor of > consolidating these. It looks like a lot of simplification is possible if we combine those. Since this is orthogonal I'd rather do it in a follow up. https://codereview.chromium.org/2201443002/diff/1/ui/chromeos/network/network... ui/chromeos/network/network_icon.cc:395: // TODO(estade): share this alpha with other things in ash (battery, etc.). On 2016/07/29 21:43:29, tdanderson wrote: > Consider referencing crbug.com/623987 Done.
estade@chromium.org changed reviewers: + bsalomon@google.com, reed@google.com
+reed/bsalomon for added DEPS dependency
lgtm btw -- does anyone ever not approve adding their lib to a DEPS? I'm not sure what I should "review" here.
On 2016/07/29 22:37:25, reed1 wrote: > lgtm > > btw -- does anyone ever not approve adding their lib to a DEPS? I'm not sure > what I should "review" here. I could see it being more of a problem for non third_party DEPS, i.e. ones where a dependency could potentially be circular. ¯\_(ツ)_/¯
The CQ bit was checked by estade@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tdanderson@chromium.org, stevenjb@chromium.org Link to the patchset: https://codereview.chromium.org/2201443002/#ps20001 (title: ".")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Md/Vectorize cellular network iconography. Side note: if we had just used .icon files, besides the massive proliferation of them that would have been necessary, we'd have worse rendering at fractional scales like 1.5x because we'd be filling fractional pixels. However, to actually get the better drawing at fractional scales, the network technology badges (LTE, G, etc.) will also have to be vectorized. BUG=617306 ========== to ========== Md/Vectorize cellular network iconography. Side note: if we had just used .icon files, besides the massive proliferation of them that would have been necessary, we'd have worse rendering at fractional scales like 1.5x because we'd be filling fractional pixels. However, to actually get the better drawing at fractional scales, the network technology badges (LTE, G, etc.) will also have to be vectorized. BUG=617306 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Md/Vectorize cellular network iconography. Side note: if we had just used .icon files, besides the massive proliferation of them that would have been necessary, we'd have worse rendering at fractional scales like 1.5x because we'd be filling fractional pixels. However, to actually get the better drawing at fractional scales, the network technology badges (LTE, G, etc.) will also have to be vectorized. BUG=617306 ========== to ========== Md/Vectorize cellular network iconography. Side note: if we had just used .icon files, besides the massive proliferation of them that would have been necessary, we'd have worse rendering at fractional scales like 1.5x because we'd be filling fractional pixels. However, to actually get the better drawing at fractional scales, the network technology badges (LTE, G, etc.) will also have to be vectorized. BUG=617306 Committed: https://crrev.com/b82f940c45a71ce43624cd915ea9a62c838ca90c Cr-Commit-Position: refs/heads/master@{#408803} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/b82f940c45a71ce43624cd915ea9a62c838ca90c Cr-Commit-Position: refs/heads/master@{#408803} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
