[Ash] Remove pre-MD code paths from HoverHighlightView

[Ash] Remove pre-MD code paths from HoverHighlightView

This CL removes all mention of ash::MaterialDesignController
from the HoverHighlightView class and performs some
preliminary cleanup of dead code. Further refactoring
and code cleanup for this class requires some additional
investigation and is tracked by crbug.com/708190.

BUG=686345
TEST=manual

Review-Url: https://codereview.chromium.org/2795143005
Cr-Commit-Position: refs/heads/master@{#462064}
Committed: https://chromium.googlesource.com/chromium/src/+/889fe85b1ec6deaef84f7c1b3a186aa0a7a3f7f5

[tdanderson, 2017-04-04 16:49:06]

The CQ bit was checked by tdanderson@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-04 16:49:42]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tdanderson, 2017-04-04 16:54:42]

tdanderson@chromium.org changed reviewers:
+ estade@chromium.org

[tdanderson, 2017-04-04 16:54:43]

Evan, can you please take a look? I recognize there is still code that needs to
be cleaned up (above and beyond just the TODOs I added), and I'm tracking as
crbug.com/708190. As a first step this removes mention of MDC and does a bit of
initial cleanup.

[Evan Stade, 2017-04-04 17:18:18]

mostly lg

in general I find it easier not to make this too piecemeal because it's easier
to find unused/unnecessary things as you're removing or modifying code than to
go through existing code and try to find what's unused. If you have specific
ideas about what to fix or simplify it would be good to write that in the bug
you filed.

https://codereview.chromium.org/2795143005/diff/1/ash/common/system/chromeos/...
File ash/common/system/chromeos/network/network_info.h (right):

https://codereview.chromium.org/2795143005/diff/1/ash/common/system/chromeos/...
ash/common/system/chromeos/network/network_info.h:21: // TODO(tdanderson):
Remove the |highlight| member. See crbug.com/708190.
why add a TODO instead of removing right away? Is there some complexity to
removing it?

https://codereview.chromium.org/2795143005/diff/1/ash/common/system/tray/hove...
File ash/common/system/tray/hover_highlight_view.cc (right):

https://codereview.chromium.org/2795143005/diff/1/ash/common/system/tray/hove...
ash/common/system/tray/hover_highlight_view.cc:64: DCHECK(tri_view_);
nit: chromium style suggests not DCHECKing in this situation as L68/69 makes it
obvious that tri_view_ is assumed to be valid.

https://codereview.chromium.org/2795143005/diff/1/ash/common/system/tray_acce...
File ash/common/system/tray_accessibility.cc (right):

https://codereview.chromium.org/2795143005/diff/1/ash/common/system/tray_acce...
ash/common/system/tray_accessibility.cc:234: bundle.GetLocalizedString(
l10n_util::GetStringUTF16()

[commit-bot: I haz the power, 2017-04-04 17:22:29]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-04 17:22:30]

Dry run: This issue passed the CQ dry run.

[tdanderson, 2017-04-04 18:43:14]

The CQ bit was checked by tdanderson@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-04 18:44:14]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tdanderson, 2017-04-04 18:45:06]

Please take another look. I have enumerated the additional cleanup work in
crbug.com/708190.

https://codereview.chromium.org/2795143005/diff/1/ash/common/system/chromeos/...
File ash/common/system/chromeos/network/network_info.h (right):

https://codereview.chromium.org/2795143005/diff/1/ash/common/system/chromeos/...
ash/common/system/chromeos/network/network_info.h:21: // TODO(tdanderson):
Remove the |highlight| member. See crbug.com/708190.
On 2017/04/04 17:18:18, Evan Stade wrote:
> why add a TODO instead of removing right away? Is there some complexity to
> removing it?

It looked like it is still used in network ordering logic. Changing that seems
out of scope for this CL and has the potential to introduce an unrelated
regression since it's not 100% a matter of just deleting dead code. For those
reasons I think it is better to land in a separate CL.

https://codereview.chromium.org/2795143005/diff/1/ash/common/system/tray/hove...
File ash/common/system/tray/hover_highlight_view.cc (right):

https://codereview.chromium.org/2795143005/diff/1/ash/common/system/tray/hove...
ash/common/system/tray/hover_highlight_view.cc:64: DCHECK(tri_view_);
On 2017/04/04 17:18:18, Evan Stade wrote:
> nit: chromium style suggests not DCHECKing in this situation as L68/69 makes
it
> obvious that tri_view_ is assumed to be valid.

Agreed, and done. However I don't see any explicit mention of this in the style
guide (either the chromium C++ one or the Google C++ one). For my own future
reference can you point me to the guide you're looking at?

https://codereview.chromium.org/2795143005/diff/1/ash/common/system/tray_acce...
File ash/common/system/tray_accessibility.cc (right):

https://codereview.chromium.org/2795143005/diff/1/ash/common/system/tray_acce...
ash/common/system/tray_accessibility.cc:234: bundle.GetLocalizedString(
On 2017/04/04 17:18:18, Evan Stade wrote:
> l10n_util::GetStringUTF16()

Done.

[commit-bot: I haz the power, 2017-04-04 19:50:13]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-04 19:50:14]

Dry run: This issue passed the CQ dry run.

[Evan Stade, 2017-04-05 13:56:43]

thanks for that list in the bug. LGTM

https://codereview.chromium.org/2795143005/diff/1/ash/common/system/tray/hove...
File ash/common/system/tray/hover_highlight_view.cc (right):

https://codereview.chromium.org/2795143005/diff/1/ash/common/system/tray/hove...
ash/common/system/tray/hover_highlight_view.cc:64: DCHECK(tri_view_);
On 2017/04/04 18:45:06, tdanderson wrote:
> On 2017/04/04 17:18:18, Evan Stade wrote:
> > nit: chromium style suggests not DCHECKing in this situation as L68/69 makes
> it
> > obvious that tri_view_ is assumed to be valid.
> 
> Agreed, and done. However I don't see any explicit mention of this in the
style
> guide (either the chromium C++ one or the Google C++ one). For my own future
> reference can you point me to the guide you're looking at?

I guess it doesn't say this directly. It just says don't handle DCHECK failures.
But glad we agree.

https://codereview.chromium.org/2795143005/diff/20001/ash/common/system/tray_...
File ash/common/system/tray_accessibility.cc (left):

https://codereview.chromium.org/2795143005/diff/20001/ash/common/system/tray_...
ash/common/system/tray_accessibility.cc:150:
ResourceBundle::GetSharedInstance().GetImageSkiaNamed(IDR_MESSAGE_CLOSE));
add TODO or file bug to fix this?

[tdanderson, 2017-04-05 14:24:39]

https://codereview.chromium.org/2795143005/diff/20001/ash/common/system/tray_...
File ash/common/system/tray_accessibility.cc (left):

https://codereview.chromium.org/2795143005/diff/20001/ash/common/system/tray_...
ash/common/system/tray_accessibility.cc:150:
ResourceBundle::GetSharedInstance().GetImageSkiaNamed(IDR_MESSAGE_CLOSE));
On 2017/04/05 13:56:43, Evan Stade wrote:
> add TODO or file bug to fix this?

AccessibilityPopupView will be ripped out as part of crbug.com/685845

[tdanderson, 2017-04-05 14:24:48]

The CQ bit was checked by tdanderson@chromium.org

[commit-bot: I haz the power, 2017-04-05 14:25:10]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-05 14:30:42]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1491402288632680,
"parent_rev": "d8b12e10d8ad68dda1a2b19b42e2366541e61f70", "commit_rev":
"889fe85b1ec6deaef84f7c1b3a186aa0a7a3f7f5"}

[commit-bot: I haz the power, 2017-04-05 14:31:27]

Description was changed from

==========
[Ash] Remove pre-MD code paths from HoverHighlightView

This CL removes all mention of ash::MaterialDesignController
from the HoverHighlightView class and performs some
preliminary cleanup of dead code. Further refactoring
and code cleanup for this class requires some additional
investigation and is tracked by crbug.com/708190.

BUG=686345
TEST=manual
==========

to

==========
[Ash] Remove pre-MD code paths from HoverHighlightView

This CL removes all mention of ash::MaterialDesignController
from the HoverHighlightView class and performs some
preliminary cleanup of dead code. Further refactoring
and code cleanup for this class requires some additional
investigation and is tracked by crbug.com/708190.

BUG=686345
TEST=manual

Review-Url: https://codereview.chromium.org/2795143005
Cr-Commit-Position: refs/heads/master@{#462064}
Committed:
https://chromium.googlesource.com/chromium/src/+/889fe85b1ec6deaef84f7c1b3a18...
==========

[commit-bot: I haz the power, 2017-04-05 14:31:28]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/889fe85b1ec6deaef84f7c1b3a18...