Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2147783002:
Add common palette tray implementation. (Closed)
DescriptionAdd common palette tray implementation.
This class will be derived from by the actual palette tools.
Design doc: go/caebn
The palette manager is added in https://codereview.chromium.org/2140343002/.
BUG=625251
Committed: https://crrev.com/9ad8a685d3f19e28ef53cdce08d6b31d82ab7910
Cr-Commit-Position: refs/heads/master@{#408492}
Patch Set 1 #Patch Set 2 : Some palette tool stubs #
Total comments: 4
Patch Set 3 : Address comments, pull parent CL changes #Patch Set 4 : Update #Patch Set 5 : Pull out individual tool stubs into separate CLs #
Total comments: 6
Patch Set 6 : Nits #
Depends on Patchset: Messages
Total messages: 30 (22 generated)
Description was changed from ========== Add some stubbed out palette tool implementations. BUG=625251 ========== to ========== Add some stubbed out palette tool implementations. The palette manager is added in https://codereview.chromium.org/2140343002/. BUG=625251 ==========
Description was changed from ========== Add some stubbed out palette tool implementations. The palette manager is added in https://codereview.chromium.org/2140343002/. BUG=625251 ========== to ========== Add some stubbed out palette tool implementations. Design doc: go/caebn The palette manager is added in https://codereview.chromium.org/2140343002/. BUG=625251 ==========
estade@chromium.org changed reviewers: + estade@chromium.org
https://codereview.chromium.org/2147783002/diff/20001/ash/common/system/chrom... File ash/common/system/chromeos/palette/palette_tool.cc (right): https://codereview.chromium.org/2147783002/diff/20001/ash/common/system/chrom... ash/common/system/chromeos/palette/palette_tool.cc:30: CreateVectorIcon(gfx::VectorIconId::CHECK, SK_ColorGREEN); Are these the right colors? If not can you add a TODO to update them? https://codereview.chromium.org/2147783002/diff/20001/ash/common/system/chrom... ash/common/system/chromeos/palette/palette_tool.cc:32: const base::string16& name = base::ASCIIToUTF16(message); this is not right, presumably message can contain non-ASCII characters. You want |message| to be a string16. Unless this entire class is just for debug or something? The docs are not sufficient for me to figure that out.
https://codereview.chromium.org/2147783002/diff/20001/ash/common/system/chrom... File ash/common/system/chromeos/palette/palette_tool.cc (right): https://codereview.chromium.org/2147783002/diff/20001/ash/common/system/chrom... ash/common/system/chromeos/palette/palette_tool.cc:30: CreateVectorIcon(gfx::VectorIconId::CHECK, SK_ColorGREEN); On 2016/07/13 18:33:55, Evan Stade wrote: > Are these the right colors? If not can you add a TODO to update them? Added TODO. https://codereview.chromium.org/2147783002/diff/20001/ash/common/system/chrom... ash/common/system/chromeos/palette/palette_tool.cc:32: const base::string16& name = base::ASCIIToUTF16(message); On 2016/07/13 18:33:55, Evan Stade wrote: > this is not right, presumably message can contain non-ASCII characters. You want > |message| to be a string16. > > Unless this entire class is just for debug or something? The docs are not > sufficient for me to figure that out. Right, these are stub tool implementations to provide a UI that looks right but doesn't actually do anything. The derived CommonPaletteTool classes are going to get replaced, but I'd like to keep most of the view logic in CommonPaletteTool.
jdufault@chromium.org changed reviewers: + oshima@chromium.org
Oshima, PTAL. Thanks!
The CQ bit was checked by jdufault@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-gn on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-gn on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...)
Description was changed from ========== Add some stubbed out palette tool implementations. Design doc: go/caebn The palette manager is added in https://codereview.chromium.org/2140343002/. BUG=625251 ========== to ========== Add common palette tray implementation. This class will be derived from by the actual palette tools. Design doc: go/caebn The palette manager is added in https://codereview.chromium.org/2140343002/. BUG=625251 ==========
The CQ bit was checked by jdufault@chromium.org to run a CQ dry run
Patchset #5 (id:80001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2147783002/diff/100001/ash/common/system/chro... File ash/common/system/chromeos/palette/common_palette_tool.h (right): https://codereview.chromium.org/2147783002/diff/100001/ash/common/system/chro... ash/common/system/chromeos/palette/common_palette_tool.h:22: // PaletteTool overrides. nit: // PaletteTool: https://codereview.chromium.org/2147783002/diff/100001/ash/common/system/chro... ash/common/system/chromeos/palette/common_palette_tool.h:28: // ViewClickListener overrides. ditto https://codereview.chromium.org/2147783002/diff/100001/ash/common/system/chro... ash/common/system/chromeos/palette/common_palette_tool.h:40: }; DISALLOW_COPY_AND_ASSIGN
The CQ bit was checked by jdufault@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2147783002/diff/100001/ash/common/system/chro... File ash/common/system/chromeos/palette/common_palette_tool.h (right): https://codereview.chromium.org/2147783002/diff/100001/ash/common/system/chro... ash/common/system/chromeos/palette/common_palette_tool.h:22: // PaletteTool overrides. On 2016/07/22 19:31:50, oshima wrote: > nit: // PaletteTool: Done. https://codereview.chromium.org/2147783002/diff/100001/ash/common/system/chro... ash/common/system/chromeos/palette/common_palette_tool.h:28: // ViewClickListener overrides. On 2016/07/22 19:31:50, oshima wrote: > ditto Done. https://codereview.chromium.org/2147783002/diff/100001/ash/common/system/chro... ash/common/system/chromeos/palette/common_palette_tool.h:40: }; On 2016/07/22 19:31:50, oshima wrote: > DISALLOW_COPY_AND_ASSIGN Done.
The CQ bit was checked by jdufault@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from oshima@chromium.org Link to the patchset: https://codereview.chromium.org/2147783002/#ps120001 (title: "Nits")
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 ========== Add common palette tray implementation. This class will be derived from by the actual palette tools. Design doc: go/caebn The palette manager is added in https://codereview.chromium.org/2140343002/. BUG=625251 ========== to ========== Add common palette tray implementation. This class will be derived from by the actual palette tools. Design doc: go/caebn The palette manager is added in https://codereview.chromium.org/2140343002/. BUG=625251 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Add common palette tray implementation. This class will be derived from by the actual palette tools. Design doc: go/caebn The palette manager is added in https://codereview.chromium.org/2140343002/. BUG=625251 ========== to ========== Add common palette tray implementation. This class will be derived from by the actual palette tools. Design doc: go/caebn The palette manager is added in https://codereview.chromium.org/2140343002/. BUG=625251 Committed: https://crrev.com/9ad8a685d3f19e28ef53cdce08d6b31d82ab7910 Cr-Commit-Position: refs/heads/master@{#408492} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/9ad8a685d3f19e28ef53cdce08d6b31d82ab7910 Cr-Commit-Position: refs/heads/master@{#408492} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
