Revert of Delete pre-MD code from OmniboxResultView

Revert of Delete pre-MD code from OmniboxResultView (patchset #9 id:160001 of https://codereview.chromium.org/2365263006/ )

Reason for revert:
The downstream Chrome on iOS code has never been ported to the new vector icon and still use MD raster icon. This CL breaks Chrome on iOS downstream (due to missing resources and removal of GetIcon() method).

Original issue's description:
> Delete pre-MD code from OmniboxResultView
>
> BUG=648281
>
> Committed: https://crrev.com/255d6d6d2bbc0e8715ad0985d239897f80a4b707
> Cr-Commit-Position: refs/heads/master@{#423690}

TBR=pkasting@chromium.org,blundell@chromium.org,sky@chromium.org,estade@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=648281
Committed: https://crrev.com/7ae88a32dfc5da7a3b0ad53f8d9ead92df14719c
Cr-Commit-Position: refs/heads/master@{#423819}

[sdefresne, 2016-10-07 08:07:59]

The CQ bit was checked by sdefresne@chromium.org

[sdefresne, 2016-10-07 08:07:59]

Created Revert of Delete pre-MD code from OmniboxResultView

[commit-bot: I haz the power, 2016-10-07 08:08:18]

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, 2016-10-07 08:10:02]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-10-07 08:11:24]

Description was changed from

==========
Revert of Delete pre-MD code from OmniboxResultView (patchset #9 id:160001 of
https://codereview.chromium.org/2365263006/ )

Reason for revert:
The downstream Chrome on iOS code has never been ported to the new vector icon
and still use MD raster icon. This CL breaks Chrome on iOS downstream (due to
missing resources and removal of GetIcon() method).

Original issue's description:
> Delete pre-MD code from OmniboxResultView
> 
> BUG=648281
> 
> Committed: https://crrev.com/255d6d6d2bbc0e8715ad0985d239897f80a4b707
> Cr-Commit-Position: refs/heads/master@{#423690}

TBR=pkasting@chromium.org,blundell@chromium.org,sky@chromium.org,estade@chrom...
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=648281
==========

to

==========
Revert of Delete pre-MD code from OmniboxResultView (patchset #9 id:160001 of
https://codereview.chromium.org/2365263006/ )

Reason for revert:
The downstream Chrome on iOS code has never been ported to the new vector icon
and still use MD raster icon. This CL breaks Chrome on iOS downstream (due to
missing resources and removal of GetIcon() method).

Original issue's description:
> Delete pre-MD code from OmniboxResultView
>
> BUG=648281
>
> Committed: https://crrev.com/255d6d6d2bbc0e8715ad0985d239897f80a4b707
> Cr-Commit-Position: refs/heads/master@{#423690}

TBR=pkasting@chromium.org,blundell@chromium.org,sky@chromium.org,estade@chrom...
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=648281

Committed: https://crrev.com/7ae88a32dfc5da7a3b0ad53f8d9ead92df14719c
Cr-Commit-Position: refs/heads/master@{#423819}
==========

[commit-bot: I haz the power, 2016-10-07 08:11:25]

Patchset 1 (id:??) landed as
https://crrev.com/7ae88a32dfc5da7a3b0ad53f8d9ead92df14719c
Cr-Commit-Position: refs/heads/master@{#423819}

[sky, 2016-10-07 15:17:39]

IMO downstream repos shouldn't stop chrome from moving forward.

What's the plan for updating IOS?

  -Scott

On Fri, Oct 7, 2016 at 1:07 AM,  <sdefresne@chromium.org> wrote:
> Reviewers: Peter Kasting (OOO Oct. 6-9), blundell, sky, Evan Stade (ooo
> friday)
> CL: https://codereview.chromium.org/2399333002/
>
> Message:
> Created Revert of Delete pre-MD code from OmniboxResultView
>
> Description:
> Revert of Delete pre-MD code from OmniboxResultView (patchset #9 id:160001
> of
> https://codereview.chromium.org/2365263006/ )
>
> Reason for revert:
> The downstream Chrome on iOS code has never been ported to the new vector
> icon
> and still use MD raster icon. This CL breaks Chrome on iOS downstream (due
> to
> missing resources and removal of GetIcon() method).
>
> Original issue's description:
>> Delete pre-MD code from OmniboxResultView
>>
>> BUG=648281
>>
>> Committed: https://crrev.com/255d6d6d2bbc0e8715ad0985d239897f80a4b707
>> Cr-Commit-Position: refs/heads/master@{#423690}
>
>
TBR=pkasting@chromium.org,blundell@chromium.org,sky@chromium.org,estade@chrom...
> # Skipping CQ checks because original CL landed less than 1 days ago.
> NOPRESUBMIT=true
> NOTREECHECKS=true
> NOTRY=true
> BUG=648281
>
> Affected files (+279, -12 lines):
> M chrome/app/theme/theme_resources.grd
> M chrome/browser/chromeos/options/network_config_view.h
> M chrome/browser/chromeos/options/network_config_view.cc
> M chrome/browser/extensions/extension_context_menu_model.cc
> M chrome/browser/ui/app_list/search/omnibox_result.cc
> M chrome/browser/ui/views/omnibox/omnibox_popup_contents_view.cc
> M chrome/browser/ui/views/omnibox/omnibox_result_view.h
> M chrome/browser/ui/views/omnibox/omnibox_result_view.cc
> M components/neterror/resources/neterror.css
> M components/omnibox/browser/autocomplete_match.h
> M components/omnibox/browser/autocomplete_match.cc
> M components/omnibox/browser/omnibox_view.h
> M components/omnibox/browser/omnibox_view.cc
> M components/resources/components_scaled_resources.grd
> D components/resources/default_100_percent/neterror/search_glass.png
> A
>
components/resources/default_100_percent/omnibox/controlled_setting_mandatory.png
> A components/resources/default_100_percent/omnibox/ios/omnibox_history.png
> A
>
components/resources/default_100_percent/omnibox/ios/omnibox_history_incognito.png
> A components/resources/default_100_percent/omnibox/ios/omnibox_http.png
> A
>
components/resources/default_100_percent/omnibox/ios/omnibox_http_incognito.png
> A
>
components/resources/default_100_percent/omnibox/ios/omnibox_https_invalid.png
> A
> components/resources/default_100_percent/omnibox/ios/omnibox_https_valid.png
> A components/resources/default_100_percent/omnibox/ios/omnibox_search.png
> A
>
components/resources/default_100_percent/omnibox/ios/omnibox_search_incognito.png
> A components/resources/default_100_percent/omnibox/location_bar_http.png
> A components/resources/default_100_percent/omnibox/omnibox_calculator.png
> A components/resources/default_100_percent/omnibox/omnibox_extension_app.png
> A components/resources/default_100_percent/omnibox/omnibox_http.png
> A components/resources/default_100_percent/omnibox/omnibox_https_invalid.png
> A components/resources/default_100_percent/omnibox/omnibox_https_valid.png
> A components/resources/default_100_percent/omnibox/omnibox_search.png
> A
>
components/resources/default_100_percent/omnibox/omnibox_search_button_loupe.png
> D components/resources/default_200_percent/neterror/search_glass.png
> A
>
components/resources/default_200_percent/omnibox/controlled_setting_mandatory.png
> A components/resources/default_200_percent/omnibox/ios/omnibox_history.png
> A
>
components/resources/default_200_percent/omnibox/ios/omnibox_history_incognito.png
> A components/resources/default_200_percent/omnibox/ios/omnibox_http.png
> A
>
components/resources/default_200_percent/omnibox/ios/omnibox_http_incognito.png
> A
>
components/resources/default_200_percent/omnibox/ios/omnibox_https_invalid.png
> A
> components/resources/default_200_percent/omnibox/ios/omnibox_https_valid.png
> A components/resources/default_200_percent/omnibox/ios/omnibox_search.png
> A
>
components/resources/default_200_percent/omnibox/ios/omnibox_search_incognito.png
> A components/resources/default_200_percent/omnibox/location_bar_http.png
> A components/resources/default_200_percent/omnibox/omnibox_extension_app.png
> A components/resources/default_200_percent/omnibox/omnibox_http.png
> A components/resources/default_200_percent/omnibox/omnibox_https_invalid.png
> A components/resources/default_200_percent/omnibox/omnibox_https_valid.png
> A components/resources/default_200_percent/omnibox/omnibox_search.png
> A
>
components/resources/default_200_percent/omnibox/omnibox_search_button_loupe.png
> A
>
components/resources/default_300_percent/omnibox/controlled_setting_mandatory.png
> A components/resources/default_300_percent/omnibox/ios/omnibox_history.png
> A
>
components/resources/default_300_percent/omnibox/ios/omnibox_history_incognito.png
> A components/resources/default_300_percent/omnibox/ios/omnibox_http.png
> A
>
components/resources/default_300_percent/omnibox/ios/omnibox_http_incognito.png
> A
>
components/resources/default_300_percent/omnibox/ios/omnibox_https_invalid.png
> A
> components/resources/default_300_percent/omnibox/ios/omnibox_https_valid.png
> A components/resources/default_300_percent/omnibox/ios/omnibox_search.png
> A
>
components/resources/default_300_percent/omnibox/ios/omnibox_search_incognito.png
> A components/resources/default_300_percent/omnibox/omnibox_extension_app.png
> A components/resources/omnibox_scaled_resources.grdp
> M components/toolbar/test_toolbar_model.h
> M components/toolbar/test_toolbar_model.cc
> M components/toolbar/toolbar_model.h
> M components/toolbar/toolbar_model_impl.h
> M components/toolbar/toolbar_model_impl.cc
>
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[sdefresne, 2016-10-07 15:27:03]

iOS has never supported gfx vector images (the corresponding support in skia is
currently inexistent). I've contacted estade@ directly to see how we can support
vector image on iOS and to get involved in the effort to move to vector images.
We usually avoid reverting upstream CL but given that this landed just before
M55 branched and adding the support for vector image on iOS is estimated to take
at least multiple engineer day of effort, I decided to revert it.

Please note that some of the resources removed are iOS specific (all those in
components/resources/default_xxx_percent/ios) and should probably not have been
removed without contacting someone familiar with the iOS code.

If we really want to remove this code, can I suggest keeping the iOS specific
resources intact and putting keeping the code that is removed from the component
behind "#if defined(OS_IOS)" until we have support for vector images on iOS. If
this is an acceptable plan, I can contribute an updated version of the CL doing
just that.

[sky, 2016-10-07 16:36:55]

If these resources are used by ios, and are part of the normal chrome
build, how come Evan's change didn't fail a try run? If these
resources aren't used by any of the code in the chrome repo or try
bots, then should it be moved downstream?

  -Scott

On Fri, Oct 7, 2016 at 8:27 AM,  <sdefresne@chromium.org> wrote:
> iOS has never supported gfx vector images (the corresponding support in skia
> is
> currently inexistent). I've contacted estade@ directly to see how we can
> support
> vector image on iOS and to get involved in the effort to move to vector
> images.
> We usually avoid reverting upstream CL but given that this landed just
> before
> M55 branched and adding the support for vector image on iOS is estimated to
> take
> at least multiple engineer day of effort, I decided to revert it.
>
> Please note that some of the resources removed are iOS specific (all those
> in
> components/resources/default_xxx_percent/ios) and should probably not have
> been
> removed without contacting someone familiar with the iOS code.
>
> If we really want to remove this code, can I suggest keeping the iOS
> specific
> resources intact and putting keeping the code that is removed from the
> component
> behind "#if defined(OS_IOS)" until we have support for vector images on iOS.
> If
> this is an acceptable plan, I can contribute an updated version of the CL
> doing
> just that.
>
> https://codereview.chromium.org/2399333002/

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[blundell, 2016-10-10 08:22:34]

Hey Scott,

On 2016/10/07 16:36:55, sky wrote:
> If these resources are used by ios, and are part of the normal chrome
> build, how come Evan's change didn't fail a try run?

The resources are currently only used downstream on iOS AFAIK.

> If these
> resources aren't used by any of the code in the chrome repo or try
> bots, then should it be moved downstream?
> 

That would require restructuring of the //components-level code, from a quick
look-through. As the team is actively working on upstreaming the iOS port, there
will be situations like this where there's code that's built upstream on iOS but
currently only used downstream (but in the medium term will be used upstream).
The team tries very hard to limit the impact of that fact on Chromium. This
particular case was tricky for the reasons that Sylvain mentioned. As far as
what the "right" next step is here, I defer to Sylvain, who is discussing with
Evan and rohitrao@ (the iOS omnibox expert).

>   -Scott
> 
> On Fri, Oct 7, 2016 at 8:27 AM,  <mailto:sdefresne@chromium.org> wrote:
> > iOS has never supported gfx vector images (the corresponding support in skia
> > is
> > currently inexistent). I've contacted estade@ directly to see how we can
> > support
> > vector image on iOS and to get involved in the effort to move to vector
> > images.
> > We usually avoid reverting upstream CL but given that this landed just
> > before
> > M55 branched and adding the support for vector image on iOS is estimated to
> > take
> > at least multiple engineer day of effort, I decided to revert it.
> >
> > Please note that some of the resources removed are iOS specific (all those
> > in
> > components/resources/default_xxx_percent/ios) and should probably not have
> > been
> > removed without contacting someone familiar with the iOS code.
> >
> > If we really want to remove this code, can I suggest keeping the iOS
> > specific
> > resources intact and putting keeping the code that is removed from the
> > component
> > behind "#if defined(OS_IOS)" until we have support for vector images on iOS.
> > If
> > this is an acceptable plan, I can contribute an updated version of the CL
> > doing
> > just that.
> >
> > https://codereview.chromium.org/2399333002/
> 
> -- 
> You received this message because you are subscribed to the Google Groups
> "Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.
>