Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2140343002:
Add palette tool/manager. (Closed)
DescriptionAdd palette tool/manager.
Design doc: go/caebn
This does not contain any tool implementations. There are stub tool implementations in https://codereview.chromium.org/2147783002/.
This is used in the palette tray in https://codereview.chromium.org/2148573002/.
BUG=625251
Committed: https://crrev.com/c909f8be1a6cfc6d8d55a67fe46d3ad4e2f4e383
Cr-Commit-Position: refs/heads/master@{#407553}
Patch Set 1 #Patch Set 2 : Add palette tool/manager #
Total comments: 6
Patch Set 3 : Address comments #
Total comments: 36
Patch Set 4 : Comments #
Total comments: 14
Patch Set 5 : Rebase #Patch Set 6 : Address comments #Patch Set 7 : Rework structure a bit #Patch Set 8 : Nit #
Total comments: 2
Patch Set 9 : Nit #Patch Set 10 : Remove base::Optional, use VECTOR_ICON_NONE instead #Dependent Patchsets: Messages
Total messages: 47 (28 generated)
Description was changed from ========== Add palette tool/manager. BUG=625251 ========== to ========== Add palette tool/manager. This does not contain any tool implementations. There are stub tool implementations in https://codereview.chromium.org/2147783002/. This is used in the palette tray in https://codereview.chromium.org/2148573002/. BUG=625251 ==========
Description was changed from ========== Add palette tool/manager. This does not contain any tool implementations. There are stub tool implementations in https://codereview.chromium.org/2147783002/. This is used in the palette tray in https://codereview.chromium.org/2148573002/. BUG=625251 ========== to ========== Add palette tool/manager. Design doc: go/caebn This does not contain any tool implementations. There are stub tool implementations in https://codereview.chromium.org/2147783002/. This is used in the palette tray in https://codereview.chromium.org/2148573002/. BUG=625251 ==========
Description was changed from ========== Add palette tool/manager. Design doc: go/caebn This does not contain any tool implementations. There are stub tool implementations in https://codereview.chromium.org/2147783002/. This is used in the palette tray in https://codereview.chromium.org/2148573002/. BUG=625251 ========== to ========== Add palette tool/manager. Design doc: go/caebn This does not contain any tool implementations. There are stub tool implementations in https://codereview.chromium.org/2147783002/. This is used in the palette tray in https://codereview.chromium.org/2148573002/. BUG=625251 ==========
estade@chromium.org changed reviewers: + estade@chromium.org
It would be helpful if you would consolidate these CLs a bit. It's hard to understand them all separately. For example, what is PaletteToolIdToString used for? Nothing, apparently (unless I go searching for it in another CL). https://codereview.chromium.org/2140343002/diff/20001/ash/common/system/chrom... File ash/common/system/chromeos/palette/palette_ids.h (right): https://codereview.chromium.org/2140343002/diff/20001/ash/common/system/chrom... ash/common/system/chromeos/palette/palette_ids.h:26: ASH_EXPORT std::string PaletteToolIdToString(PaletteToolId tool_id); Are these just for debug?
https://codereview.chromium.org/2140343002/diff/20001/ash/common/system/chrom... File ash/common/system/chromeos/palette/palette_tool.h (right): https://codereview.chromium.org/2140343002/diff/20001/ash/common/system/chrom... ash/common/system/chromeos/palette/palette_tool.h:28: class ASH_EXPORT PaletteTool { class needs docs https://codereview.chromium.org/2140343002/diff/20001/ash/common/system/chrom... ash/common/system/chromeos/palette/palette_tool.h:58: virtual void DestroyView() = 0; more like ViewDestroyed
https://codereview.chromium.org/2140343002/diff/20001/ash/common/system/chrom... File ash/common/system/chromeos/palette/palette_ids.h (right): https://codereview.chromium.org/2140343002/diff/20001/ash/common/system/chrom... ash/common/system/chromeos/palette/palette_ids.h:26: ASH_EXPORT std::string PaletteToolIdToString(PaletteToolId tool_id); On 2016/07/13 18:31:12, Evan Stade wrote: > Are these just for debug? Essentially, though right now the palette tool stubs are using PaletteToolIdToString for display strings - eventually though those will be properly i18nized. https://codereview.chromium.org/2140343002/diff/20001/ash/common/system/chrom... File ash/common/system/chromeos/palette/palette_tool.h (right): https://codereview.chromium.org/2140343002/diff/20001/ash/common/system/chrom... ash/common/system/chromeos/palette/palette_tool.h:28: class ASH_EXPORT PaletteTool { On 2016/07/13 18:34:01, Evan Stade wrote: > class needs docs Done. https://codereview.chromium.org/2140343002/diff/20001/ash/common/system/chrom... ash/common/system/chromeos/palette/palette_tool.h:58: virtual void DestroyView() = 0; On 2016/07/13 18:34:01, Evan Stade wrote: > more like ViewDestroyed Done - the idea was to mirror SystemTrayItem, but I also strongly prefer this name.
jdufault@chromium.org changed reviewers: + oshima@chromium.org
oshima@ - PTAL. Thanks!
https://codereview.chromium.org/2140343002/diff/40001/ash/common/system/chrom... File ash/common/system/chromeos/palette/palette_ids.cc (right): https://codereview.chromium.org/2140343002/diff/40001/ash/common/system/chrom... ash/common/system/chromeos/palette/palette_ids.cc:27: return ""; std::string() https://codereview.chromium.org/2140343002/diff/40001/ash/common/system/chrom... ash/common/system/chromeos/palette/palette_ids.cc:39: return ""; ditto https://codereview.chromium.org/2140343002/diff/40001/ash/common/system/chrom... File ash/common/system/chromeos/palette/palette_ids.h (right): https://codereview.chromium.org/2140343002/diff/40001/ash/common/system/chrom... ash/common/system/chromeos/palette/palette_ids.h:14: enum class PaletteGroup { ACTION, MODE }; Document these enums https://codereview.chromium.org/2140343002/diff/40001/ash/common/system/chrom... File ash/common/system/chromeos/palette/palette_tool.cc (right): https://codereview.chromium.org/2140343002/diff/40001/ash/common/system/chrom... ash/common/system/chromeos/palette/palette_tool.cc:16: #include "ui/gfx/vector_icons_public.h" Looks like not all headers are necessary? https://codereview.chromium.org/2140343002/diff/40001/ash/common/system/chrom... ash/common/system/chromeos/palette/palette_tool.cc:21: void PaletteTool::RegisterToolInstances(PaletteToolManager* tool_manager) {} what is this for? or add TODO? https://codereview.chromium.org/2140343002/diff/40001/ash/common/system/chrom... ash/common/system/chromeos/palette/palette_tool.cc:32: disable_tool_ = disable_tool; in initializer list? https://codereview.chromium.org/2140343002/diff/40001/ash/common/system/chrom... ash/common/system/chromeos/palette/palette_tool.cc:44: return enabled_; you may inline this. https://codereview.chromium.org/2140343002/diff/40001/ash/common/system/chrom... ash/common/system/chromeos/palette/palette_tool.cc:61: } this too https://codereview.chromium.org/2140343002/diff/40001/ash/common/system/chrom... File ash/common/system/chromeos/palette/palette_tool.h (right): https://codereview.chromium.org/2140343002/diff/40001/ash/common/system/chrom... ash/common/system/chromeos/palette/palette_tool.h:50: virtual PaletteGroup group() const = 0; new line in between https://codereview.chromium.org/2140343002/diff/40001/ash/common/system/chrom... ash/common/system/chromeos/palette/palette_tool.h:57: virtual void OnEnable(); new line in between https://codereview.chromium.org/2140343002/diff/40001/ash/common/system/chrom... ash/common/system/chromeos/palette/palette_tool.h:67: virtual void ViewDestroyed() = 0; OnViewDestroyed() should be more consistent with the rest of chromium. https://codereview.chromium.org/2140343002/diff/40001/ash/common/system/chrom... File ash/common/system/chromeos/palette/palette_tool_manager.cc (right): https://codereview.chromium.org/2140343002/diff/40001/ash/common/system/chrom... ash/common/system/chromeos/palette/palette_tool_manager.cc:69: views.reserve(tools_.size()); views(tools_.size()) https://codereview.chromium.org/2140343002/diff/40001/ash/common/system/chrom... ash/common/system/chromeos/palette/palette_tool_manager.cc:92: find_if + lambda? you may leave it if it looks more complicated. https://codereview.chromium.org/2140343002/diff/40001/ash/common/system/chrom... File ash/common/system/chromeos/palette/palette_tool_manager.h (right): https://codereview.chromium.org/2140343002/diff/40001/ash/common/system/chrom... ash/common/system/chromeos/palette/palette_tool_manager.h:48: void ActivateTool(PaletteToolId tool_id); newline before comment. same for the rest. https://codereview.chromium.org/2140343002/diff/40001/ash/common/system/chrom... ash/common/system/chromeos/palette/palette_tool_manager.h:61: void ViewsDestroyed(); NotifyViewsDestroyed ( or NotifyOnViewsDestroyed). https://codereview.chromium.org/2140343002/diff/40001/ash/common/system/chrom... File ash/common/system/chromeos/palette/palette_tool_manager_unittest.cc (right): https://codereview.chromium.org/2140343002/diff/40001/ash/common/system/chrom... ash/common/system/chromeos/palette/palette_tool_manager_unittest.cc:40: }; DISALLOW_COPY_AND_ASSIGN https://codereview.chromium.org/2140343002/diff/40001/ash/common/system/chrom... ash/common/system/chromeos/palette/palette_tool_manager_unittest.cc:61: DCHECK(tool->tool_id() != tool_id); DCHECK_NE https://codereview.chromium.org/2140343002/diff/40001/ash/common/system/chrom... ash/common/system/chromeos/palette/palette_tool_manager_unittest.cc:84: TestTool* action_1 = tools_[tools_.size() - 1]; tools_.back() ?
The CQ bit was checked by jdufault@chromium.org to run a CQ dry run
https://codereview.chromium.org/2140343002/diff/40001/ash/common/system/chrom... File ash/common/system/chromeos/palette/palette_ids.cc (right): https://codereview.chromium.org/2140343002/diff/40001/ash/common/system/chrom... ash/common/system/chromeos/palette/palette_ids.cc:27: return ""; On 2016/07/14 13:58:31, oshima wrote: > std::string() Done. https://codereview.chromium.org/2140343002/diff/40001/ash/common/system/chrom... ash/common/system/chromeos/palette/palette_ids.cc:39: return ""; On 2016/07/14 13:58:31, oshima wrote: > ditto Done. https://codereview.chromium.org/2140343002/diff/40001/ash/common/system/chrom... File ash/common/system/chromeos/palette/palette_ids.h (right): https://codereview.chromium.org/2140343002/diff/40001/ash/common/system/chrom... ash/common/system/chromeos/palette/palette_ids.h:14: enum class PaletteGroup { ACTION, MODE }; On 2016/07/14 13:58:32, oshima wrote: > Document these enums Done. https://codereview.chromium.org/2140343002/diff/40001/ash/common/system/chrom... File ash/common/system/chromeos/palette/palette_tool.cc (right): https://codereview.chromium.org/2140343002/diff/40001/ash/common/system/chrom... ash/common/system/chromeos/palette/palette_tool.cc:16: #include "ui/gfx/vector_icons_public.h" On 2016/07/14 13:58:32, oshima wrote: > Looks like not all headers are necessary? Done, these are used in a dependent CL so I've moved them to that CL. https://codereview.chromium.org/2140343002/diff/40001/ash/common/system/chrom... ash/common/system/chromeos/palette/palette_tool.cc:21: void PaletteTool::RegisterToolInstances(PaletteToolManager* tool_manager) {} On 2016/07/14 13:58:32, oshima wrote: > what is this for? or add TODO? Added a comment. https://codereview.chromium.org/2140343002/diff/40001/ash/common/system/chrom... ash/common/system/chromeos/palette/palette_tool.cc:32: disable_tool_ = disable_tool; On 2016/07/14 13:58:32, oshima wrote: > in initializer list? Not a constructor :( https://codereview.chromium.org/2140343002/diff/40001/ash/common/system/chrom... ash/common/system/chromeos/palette/palette_tool.cc:44: return enabled_; On 2016/07/14 13:58:32, oshima wrote: > you may inline this. Done. https://codereview.chromium.org/2140343002/diff/40001/ash/common/system/chrom... ash/common/system/chromeos/palette/palette_tool.cc:61: } On 2016/07/14 13:58:32, oshima wrote: > this too Done. https://codereview.chromium.org/2140343002/diff/40001/ash/common/system/chrom... File ash/common/system/chromeos/palette/palette_tool.h (right): https://codereview.chromium.org/2140343002/diff/40001/ash/common/system/chrom... ash/common/system/chromeos/palette/palette_tool.h:50: virtual PaletteGroup group() const = 0; On 2016/07/14 13:58:32, oshima wrote: > new line in between Done. https://codereview.chromium.org/2140343002/diff/40001/ash/common/system/chrom... ash/common/system/chromeos/palette/palette_tool.h:57: virtual void OnEnable(); On 2016/07/14 13:58:32, oshima wrote: > new line in between Done. https://codereview.chromium.org/2140343002/diff/40001/ash/common/system/chrom... ash/common/system/chromeos/palette/palette_tool.h:67: virtual void ViewDestroyed() = 0; On 2016/07/14 13:58:32, oshima wrote: > OnViewDestroyed() should be more consistent with the rest of chromium. Done. https://codereview.chromium.org/2140343002/diff/40001/ash/common/system/chrom... File ash/common/system/chromeos/palette/palette_tool_manager.cc (right): https://codereview.chromium.org/2140343002/diff/40001/ash/common/system/chrom... ash/common/system/chromeos/palette/palette_tool_manager.cc:69: views.reserve(tools_.size()); On 2016/07/14 13:58:32, oshima wrote: > views(tools_.size()) > Done. This is actually equivalent to views.resize(tools_size()), so I've modified the for loop so it directly updates the allocated memory. I prefer the previous approach since it plays with memory less. https://codereview.chromium.org/2140343002/diff/40001/ash/common/system/chrom... ash/common/system/chromeos/palette/palette_tool_manager.cc:92: On 2016/07/14 13:58:32, oshima wrote: > find_if + lambda? you may leave it if it looks more complicated. Here's find_if. I prefer the for loop. auto result = std::find_if(tools_.begin(), tools_.end(), [tool_id] (std::unique_ptr<PaletteTool>& tool) { return tool->tool_id() == tool_id; }); DCHECK(result != tools_.end()); return result->get(); https://codereview.chromium.org/2140343002/diff/40001/ash/common/system/chrom... File ash/common/system/chromeos/palette/palette_tool_manager.h (right): https://codereview.chromium.org/2140343002/diff/40001/ash/common/system/chrom... ash/common/system/chromeos/palette/palette_tool_manager.h:48: void ActivateTool(PaletteToolId tool_id); On 2016/07/14 13:58:32, oshima wrote: > newline before comment. same for the rest. Done. https://codereview.chromium.org/2140343002/diff/40001/ash/common/system/chrom... ash/common/system/chromeos/palette/palette_tool_manager.h:61: void ViewsDestroyed(); On 2016/07/14 13:58:32, oshima wrote: > NotifyViewsDestroyed ( or NotifyOnViewsDestroyed). Done. https://codereview.chromium.org/2140343002/diff/40001/ash/common/system/chrom... File ash/common/system/chromeos/palette/palette_tool_manager_unittest.cc (right): https://codereview.chromium.org/2140343002/diff/40001/ash/common/system/chrom... ash/common/system/chromeos/palette/palette_tool_manager_unittest.cc:40: }; On 2016/07/14 13:58:32, oshima wrote: > DISALLOW_COPY_AND_ASSIGN Done. https://codereview.chromium.org/2140343002/diff/40001/ash/common/system/chrom... ash/common/system/chromeos/palette/palette_tool_manager_unittest.cc:61: DCHECK(tool->tool_id() != tool_id); On 2016/07/14 13:58:32, oshima wrote: > DCHECK_NE Done. https://codereview.chromium.org/2140343002/diff/40001/ash/common/system/chrom... ash/common/system/chromeos/palette/palette_tool_manager_unittest.cc:84: TestTool* action_1 = tools_[tools_.size() - 1]; On 2016/07/14 13:58:32, oshima wrote: > tools_.back() ? Ah, much better! Thanks
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/...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
FYI, build failures are caused because a rebase is needed.
https://codereview.chromium.org/2140343002/diff/60001/ash/common/system/chrom... File ash/common/system/chromeos/palette/palette_tool.h (right): https://codereview.chromium.org/2140343002/diff/60001/ash/common/system/chrom... ash/common/system/chromeos/palette/palette_tool.h:51: virtual PaletteGroup group() const = 0; nit: GetGroup() https://codereview.chromium.org/2140343002/diff/60001/ash/common/system/chrom... ash/common/system/chromeos/palette/palette_tool.h:55: virtual PaletteToolId tool_id() const = 0; nit: GetToolId() https://codereview.chromium.org/2140343002/diff/60001/ash/common/system/chrom... ash/common/system/chromeos/palette/palette_tool.h:80: WmWindow* window() const { return window_; } a method that returns internal state as non const should be non const. https://codereview.chromium.org/2140343002/diff/60001/ash/common/system/chrom... ash/common/system/chromeos/palette/palette_tool.h:87: }; DISALLOW_COPY_AND_ASSIGN https://codereview.chromium.org/2140343002/diff/60001/ash/common/system/chrom... File ash/common/system/chromeos/palette/palette_tool_manager.cc (right): https://codereview.chromium.org/2140343002/diff/60001/ash/common/system/chrom... ash/common/system/chromeos/palette/palette_tool_manager.cc:62: if (!active_tool) nit: ternary operator? https://codereview.chromium.org/2140343002/diff/60001/ash/common/system/chrom... File ash/common/system/chromeos/palette/palette_tool_manager_unittest.cc (right): https://codereview.chromium.org/2140343002/diff/60001/ash/common/system/chrom... ash/common/system/chromeos/palette/palette_tool_manager_unittest.cc:66: tools_.push_back(tool); I think it's better to just keep get the pointer from unique_ptr in the body, and add DCHECK in the manager's AddTool (using find_if + lambda).
The CQ bit was checked by jdufault@chromium.org to run a CQ dry run
https://codereview.chromium.org/2140343002/diff/60001/ash/common/system/chrom... File ash/common/system/chromeos/palette/palette_tool.h (right): https://codereview.chromium.org/2140343002/diff/60001/ash/common/system/chrom... ash/common/system/chromeos/palette/palette_tool.h:51: virtual PaletteGroup group() const = 0; On 2016/07/19 00:08:53, oshima wrote: > nit: GetGroup() Done. https://codereview.chromium.org/2140343002/diff/60001/ash/common/system/chrom... ash/common/system/chromeos/palette/palette_tool.h:55: virtual PaletteToolId tool_id() const = 0; On 2016/07/19 00:08:53, oshima wrote: > nit: GetToolId() Done. https://codereview.chromium.org/2140343002/diff/60001/ash/common/system/chrom... ash/common/system/chromeos/palette/palette_tool.h:80: WmWindow* window() const { return window_; } On 2016/07/19 00:08:53, oshima wrote: > a method that returns internal state as non const should be non const. Done. https://codereview.chromium.org/2140343002/diff/60001/ash/common/system/chrom... ash/common/system/chromeos/palette/palette_tool.h:87: }; On 2016/07/19 00:08:53, oshima wrote: > DISALLOW_COPY_AND_ASSIGN Done. https://codereview.chromium.org/2140343002/diff/60001/ash/common/system/chrom... File ash/common/system/chromeos/palette/palette_tool_manager.cc (right): https://codereview.chromium.org/2140343002/diff/60001/ash/common/system/chrom... ash/common/system/chromeos/palette/palette_tool_manager.cc:62: if (!active_tool) On 2016/07/19 00:08:53, oshima wrote: > nit: ternary operator? Done. https://codereview.chromium.org/2140343002/diff/60001/ash/common/system/chrom... File ash/common/system/chromeos/palette/palette_tool_manager_unittest.cc (right): https://codereview.chromium.org/2140343002/diff/60001/ash/common/system/chrom... ash/common/system/chromeos/palette/palette_tool_manager_unittest.cc:66: tools_.push_back(tool); On 2016/07/19 00:08:53, oshima wrote: > I think it's better to just keep get the pointer from unique_ptr in the body, > and add DCHECK in the manager's AddTool (using find_if + lambda). > I moved the DCHECK into PaletteToolManager::AddTool. > keep get the pointer from unique_ptr in the body Can you clarify this?
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.
Oshima, ping / PTAL. I've also changed a bit of the code around to use Delegates instead of callbacks since the number of callbacks was getting unwieldy.
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/2140343002/diff/60001/ash/common/system/chrom... File ash/common/system/chromeos/palette/palette_tool_manager_unittest.cc (right): https://codereview.chromium.org/2140343002/diff/60001/ash/common/system/chrom... ash/common/system/chromeos/palette/palette_tool_manager_unittest.cc:66: tools_.push_back(tool); On 2016/07/19 18:06:21, jdufault wrote: > On 2016/07/19 00:08:53, oshima wrote: > > I think it's better to just keep get the pointer from unique_ptr in the body, > > and add DCHECK in the manager's AddTool (using find_if + lambda). > > > > I moved the DCHECK into PaletteToolManager::AddTool. > > > keep get the pointer from unique_ptr in the body > > Can you clarify this std::unique_ptr<TestTool> tool_1(new TestTool....); TestTool* test_tool_ptr(tool_1.get()); palette_tool_manager_->AddTool(std::move(tool_1)); seems to be simpler and actually require less code, than passing pointer via std::vector.
https://codereview.chromium.org/2140343002/diff/60001/ash/common/system/chrom... File ash/common/system/chromeos/palette/palette_tool_manager_unittest.cc (right): https://codereview.chromium.org/2140343002/diff/60001/ash/common/system/chrom... ash/common/system/chromeos/palette/palette_tool_manager_unittest.cc:66: tools_.push_back(tool); On 2016/07/22 19:27:51, oshima wrote: > On 2016/07/19 18:06:21, jdufault wrote: > > On 2016/07/19 00:08:53, oshima wrote: > > > I think it's better to just keep get the pointer from unique_ptr in the > body, > > > and add DCHECK in the manager's AddTool (using find_if + lambda). > > > > > > > I moved the DCHECK into PaletteToolManager::AddTool. > > > > > keep get the pointer from unique_ptr in the body > > > > Can you clarify this > > std::unique_ptr<TestTool> tool_1(new TestTool....); > TestTool* test_tool_ptr(tool_1.get()); > palette_tool_manager_->AddTool(std::move(tool_1)); > > seems to be simpler and actually require less code, than passing pointer via > std::vector. Done. I allocated directly to the TestTool* pointer to remove a line, but if preferred I can allocate directly to a std::unique_ptr and add the unique_ptr.get() call. I can also remove this function if you prefer and inline all of the calls.
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...
lgtm https://codereview.chromium.org/2140343002/diff/140001/ash/common/system/chro... File ash/common/system/chromeos/palette/palette_tool_manager_unittest.cc (right): https://codereview.chromium.org/2140343002/diff/140001/ash/common/system/chro... ash/common/system/chromeos/palette/palette_tool_manager_unittest.cc:69: auto tool = new TestTool(this, group, tool_id); auto* tool
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2140343002/diff/140001/ash/common/system/chro... File ash/common/system/chromeos/palette/palette_tool_manager_unittest.cc (right): https://codereview.chromium.org/2140343002/diff/140001/ash/common/system/chro... ash/common/system/chromeos/palette/palette_tool_manager_unittest.cc:69: auto tool = new TestTool(this, group, tool_id); On 2016/07/22 20:43:16, oshima wrote: > auto* tool 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/2140343002/#ps160001 (title: "Nit")
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
Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
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/2140343002/#ps180001 (title: "Remove base::Optional, use VECTOR_ICON_NONE instead")
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 palette tool/manager. Design doc: go/caebn This does not contain any tool implementations. There are stub tool implementations in https://codereview.chromium.org/2147783002/. This is used in the palette tray in https://codereview.chromium.org/2148573002/. BUG=625251 ========== to ========== Add palette tool/manager. Design doc: go/caebn This does not contain any tool implementations. There are stub tool implementations in https://codereview.chromium.org/2147783002/. This is used in the palette tray in https://codereview.chromium.org/2148573002/. BUG=625251 ==========
Message was sent while issue was closed.
Committed patchset #10 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== Add palette tool/manager. Design doc: go/caebn This does not contain any tool implementations. There are stub tool implementations in https://codereview.chromium.org/2147783002/. This is used in the palette tray in https://codereview.chromium.org/2148573002/. BUG=625251 ========== to ========== Add palette tool/manager. Design doc: go/caebn This does not contain any tool implementations. There are stub tool implementations in https://codereview.chromium.org/2147783002/. This is used in the palette tray in https://codereview.chromium.org/2148573002/. BUG=625251 Committed: https://crrev.com/c909f8be1a6cfc6d8d55a67fe46d3ad4e2f4e383 Cr-Commit-Position: refs/heads/master@{#407553} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/c909f8be1a6cfc6d8d55a67fe46d3ad4e2f4e383 Cr-Commit-Position: refs/heads/master@{#407553} |
