Omnibox: Remove Hint Text Code

Omnibox: Remove Hint Text Code

and associated field trial and delete unused strings.

While at it, rename
IDS_OMNIBOX_EMPTY_HINT_WITH_DEFAULT_SEARCH_PROVIDER
to
IDS_SEARCH_BOX_EMPTY_HINT_WITH_DEFAULT_SEARCH_PROVIDER

TBR=kmadhusu
(for trivial rename change in chrome/browser/search/...)

BUG=447809
Committed: https://crrev.com/c9f46539657b1006216a026a1b5b7a6b52857f3a
Cr-Commit-Position: refs/heads/master@{#326932}

[Mark P, 2015-04-22 16:00:07]

mpearson@chromium.org changed reviewers:
+ groby@chromium.org, pkasting@chromium.org

[Mark P, 2015-04-22 16:01:24]

Hi reviewers,

Please take a look.
pkasting - full review.
groby - review to make sure I deleted all the hint text / placeholder text logic
that was part of the origin chip experiment.  (i.e., is there more logic
elsewhere that was added by is no longer necessary?)

thanks,
mark

[groby-ooo-7-16, 2015-04-22 18:25:09]

c/b/ui/cocoa is LGTM

Meanwhile, IDS_OMNIBOX_EMPTY_HINT_WITH_DEFAULT_SEARCH_PROVIDER could technically
go away if local_ntp_source.cc didn't refer to it. Is that by accident or
intention? (And either way, it probably shouldn't have OMNIBOX in the name any
more)

Also: can  OmniboxView::GetHintText go?

[Mark P, 2015-04-22 18:44:33]

On Wed, Apr 22, 2015 at 11:25 AM, <groby@chromium.org> wrote:

> c/b/ui/cocoa is LGTM
>
> Meanwhile, IDS_OMNIBOX_EMPTY_HINT_WITH_DEFAULT_SEARCH_PROVIDER could
> technically
> go away if local_ntp_source.cc didn't refer to it. Is that by accident or
> intention? (And either way, it probably shouldn't have OMNIBOX in the name
> any
> more)
>

I'll check with the NTP page team about whether it can go away.


> Also: can  OmniboxView::GetHintText go?
>

Yes.  I delete the code but forgot to delete the declaration.  Amazingly,
no compiler complained.

--mark


>
>
> https://codereview.chromium.org/1104513003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Peter Kasting, 2015-04-23 00:52:49]

LGTM

FWIW: It's not actually amazing that no compiler complained about your leftover
declaration -- at compile time it's legal to compile a module with unused
forward decls (or every C++ file that included a header would have to use every
symbol in the header), and at link time it's legal to link together modules
which don't contain all forward-declared symbols as long as they don't refer to
those symbols (or, again, various legal programs wouldn't link).

https://codereview.chromium.org/1104513003/diff/40001/chrome/browser/ui/omnib...
File chrome/browser/ui/omnibox/omnibox_view.cc (left):

https://codereview.chromium.org/1104513003/diff/40001/chrome/browser/ui/omnib...
chrome/browser/ui/omnibox/omnibox_view.cc:134:
IDS_OMNIBOX_EMPTY_HINT_WITH_DEFAULT_SEARCH_PROVIDER,
I don't think this string can be removed from the local NTP source, but I'd
change the name of the resource to IDS_EMPTY_SEARCH_BOX_HINT or something.

https://codereview.chromium.org/1104513003/diff/40001/chrome/browser/ui/omnib...
chrome/browser/ui/omnibox/omnibox_view.cc:140:
IDS_OMNIBOX_EMPTY_HINT_NO_DEFAULT_SEARCH_PROVIDER);
This string is now unused and should be removed.

[Mark P, 2015-04-23 21:12:41]

Unless there are additional comments, I'll submit tomorrow (with TBR for the
necessary approvers for the rename).

--mark

https://codereview.chromium.org/1104513003/diff/40001/chrome/browser/ui/omnib...
File chrome/browser/ui/omnibox/omnibox_view.cc (left):

https://codereview.chromium.org/1104513003/diff/40001/chrome/browser/ui/omnib...
chrome/browser/ui/omnibox/omnibox_view.cc:134:
IDS_OMNIBOX_EMPTY_HINT_WITH_DEFAULT_SEARCH_PROVIDER,
On 2015/04/23 00:52:49, Peter Kasting wrote:
> I don't think this string can be removed from the local NTP source, but I'd
> change the name of the resource to IDS_EMPTY_SEARCH_BOX_HINT or something.

Replaced OMNIBOX WITH SEARCH_BOX on this string.

https://codereview.chromium.org/1104513003/diff/40001/chrome/browser/ui/omnib...
chrome/browser/ui/omnibox/omnibox_view.cc:140:
IDS_OMNIBOX_EMPTY_HINT_NO_DEFAULT_SEARCH_PROVIDER);
On 2015/04/23 00:52:49, Peter Kasting wrote:
> This string is now unused and should be removed.

Done.

[Peter Kasting, 2015-04-23 21:17:00]

https://codereview.chromium.org/1104513003/diff/40001/chrome/browser/ui/omnib...
File chrome/browser/ui/omnibox/omnibox_view.cc (left):

https://codereview.chromium.org/1104513003/diff/40001/chrome/browser/ui/omnib...
chrome/browser/ui/omnibox/omnibox_view.cc:134:
IDS_OMNIBOX_EMPTY_HINT_WITH_DEFAULT_SEARCH_PROVIDER,
On 2015/04/23 21:12:41, Mark P wrote:
> On 2015/04/23 00:52:49, Peter Kasting wrote:
> > I don't think this string can be removed from the local NTP source, but I'd
> > change the name of the resource to IDS_EMPTY_SEARCH_BOX_HINT or something.
> 
> Replaced OMNIBOX WITH SEARCH_BOX on this string.

OK.  I think that leaves too verbose of a name ("WITH_DEFAULT_SEARCH_PROVIDER"
is no longer needed for disambiguation and just seems confusing), but I don't
care much.

[Mark P, 2015-04-23 21:20:02]

https://codereview.chromium.org/1104513003/diff/40001/chrome/browser/ui/omnib...
File chrome/browser/ui/omnibox/omnibox_view.cc (left):

https://codereview.chromium.org/1104513003/diff/40001/chrome/browser/ui/omnib...
chrome/browser/ui/omnibox/omnibox_view.cc:134:
IDS_OMNIBOX_EMPTY_HINT_WITH_DEFAULT_SEARCH_PROVIDER,
On 2015/04/23 21:16:59, Peter Kasting wrote:
> On 2015/04/23 21:12:41, Mark P wrote:
> > On 2015/04/23 00:52:49, Peter Kasting wrote:
> > > I don't think this string can be removed from the local NTP source, but
I'd
> > > change the name of the resource to IDS_EMPTY_SEARCH_BOX_HINT or something.
> > 
> > Replaced OMNIBOX WITH SEARCH_BOX on this string.
> 
> OK.  I think that leaves too verbose of a name ("WITH_DEFAULT_SEARCH_PROVIDER"
> is no longer needed for disambiguation and just seems confusing), but I don't
> care much.

Done.

[Mark P, 2015-04-24 21:28:47]

The CQ bit was checked by mpearson@chromium.org

[Mark P, 2015-04-24 21:28:48]

The patchset sent to the CQ was uploaded after l-g-t-m from groby@chromium.org,
pkasting@chromium.org
Link to the patchset: https://codereview.chromium.org/1104513003/#ps80001
(title: "removed WITH_DEFAULT_SEARCH_PROVIDER part of string")

[Mark P, 2015-04-24 21:28:50]

The CQ bit was unchecked by mpearson@chromium.org

[Mark P, 2015-04-24 21:30:11]

The CQ bit was checked by mpearson@chromium.org

[commit-bot: I haz the power, 2015-04-24 21:31:12]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1104513003/80001

[commit-bot: I haz the power, 2015-04-24 23:33:26]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-04-24 23:34:27]

Patchset 5 (id:??) landed as
https://crrev.com/c9f46539657b1006216a026a1b5b7a6b52857f3a
Cr-Commit-Position: refs/heads/master@{#326932}