Make Win HRESULT into link to help-center.

Make Win HRESULT into link to help-center.

In order to help users debug their own update problems, link an HRESULT directly to a relevant help-center article about that error code.

BUG=489801
Committed: https://crrev.com/5911597ed78775ad68bcff36cdda5c7214e8c03a
Cr-Commit-Position: refs/heads/master@{#336839}

[bcwhite, 2015-06-26 18:54:37]

bcwhite@chromium.org changed reviewers:
+ jhawkins@chromium.org

[bcwhite, 2015-06-26 18:55:26]

Note: Unit-test is under construction but I wanted to get feedback about how I'm
doing this before I went too far.  Thanks.

[bcwhite, 2015-06-30 16:53:53]

Patchset #3 (id:40001) has been deleted

[bcwhite, 2015-06-30 16:53:59]

Patchset #3 (id:60001) has been deleted

[bcwhite, 2015-06-30 16:57:12]

I've moved the link injection to where the original message is generated.

[James Hawkins, 2015-06-30 17:50:52]

Looks way better, thanks.

lgtm

[bcwhite, 2015-06-30 18:10:54]

The CQ bit was checked by bcwhite@chromium.org

[commit-bot: I haz the power, 2015-06-30 18:12:21]

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

[grt (UTC plus 2), 2015-06-30 18:30:30]

grt@chromium.org changed reviewers:
+ grt@chromium.org

[grt (UTC plus 2), 2015-06-30 18:30:31]

google_update_win.cc seems too far away from UI to have the link inserted. I'd
envisioned this happening in VersionUpdaterWin::OnError where it's easier to
customize the error message. It's simple to pass the HRESULT to OnError. How
does this seem?

[commit-bot: I haz the power, 2015-06-30 19:11:50]

Committed patchset #3 (id:80001)

[commit-bot: I haz the power, 2015-06-30 19:12:55]

Patchset 3 (id:??) landed as
https://crrev.com/5911597ed78775ad68bcff36cdda5c7214e8c03a
Cr-Commit-Position: refs/heads/master@{#336839}

[chromium-reviews, 2015-06-30 19:23:02]

This didn't seem to get logged in the issue...

I considered doing it there but since it's a Google Help-Center page, it
seemed reasonable to create links from only google_update_win.  I can
change it easily enough.

-- Brian


On Tue, Jun 30, 2015 at 2:30 PM, <grt@chromium.org> wrote:

> google_update_win.cc seems too far away from UI to have the link inserted.
> I'd
> envisioned this happening in VersionUpdaterWin::OnError where it's easier
> to
> customize the error message. It's simple to pass the HRESULT to OnError.
> How
> does this seem?
>
> https://codereview.chromium.org/1212783003/
>



-- 
  Brian
  bcwhite@google.com
-----------------------------------------------------------------------------------------



*Treat someone as they are and they will remain that way.Treat someone as
they can be and they will become that way.*

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

[grt (UTC plus 2), 2015-06-30 20:35:40]

On 2015/06/30 19:23:02, chromium-reviews wrote:
> This didn't seem to get logged in the issue...
> 
> I considered doing it there but since it's a Google Help-Center page, it
> seemed reasonable to create links from only google_update_win.  I can
> change it easily enough.

Please do. It doesn't feel right to me to have html injected at this layer.
Thanks.

I also wonder about the message itself. I think we may want some friendly text
around the link. Something telling the usre that they may be able to get help if
they follow the link. I'm not a UX expert, though, so please get feedback on the
bug. My understanding is that someone should chime in in response to the
Review-UI label.

> -- Brian
> 
> 
> On Tue, Jun 30, 2015 at 2:30 PM, <mailto:grt@chromium.org> wrote:
> 
> > google_update_win.cc seems too far away from UI to have the link inserted.
> > I'd
> > envisioned this happening in VersionUpdaterWin::OnError where it's easier
> > to
> > customize the error message. It's simple to pass the HRESULT to OnError.
> > How
> > does this seem?

[bcwhite, 2015-06-30 21:29:03]

> Please do. It doesn't feel right to me to have html injected at this layer.
> Thanks.

Taking a closer look...  The internationalization is occurring within
google_update, including the inclusion of the error code (and link).  If I move
link-injection to version_updater, I have to also move the i18n code, move the
error code outside of the i18n string, or make the entire message a link.  What
do you think?


> I also wonder about the message itself. I think we may want some friendly text
> around the link. Something telling the usre that they may be able to get help
if
> they follow the link. I'm not a UX expert, though, so please get feedback on
the
> bug. My understanding is that someone should chime in in response to the
> Review-UI label.

We'll see what they say.  Personally I think the error code appearing in blue is
a pretty obvious signal of "more info here" but there may be UX policies towards
a different method.

[grt (UTC plus 2), 2015-07-01 18:26:00]

On 2015/06/30 21:29:03, bcwhite wrote:
> > Please do. It doesn't feel right to me to have html injected at this layer.
> > Thanks.
> 
> Taking a closer look...  The internationalization is occurring within
> google_update, including the inclusion of the error code (and link).  If I
move
> link-injection to version_updater, I have to also move the i18n code, move the
> error code outside of the i18n string, or make the entire message a link. 
What
> do you think?

Wow, this is a mess. There's some string building in
UpdateCheckDriver::OnUpgradeError and some in VersionUpdaterWin::OnError. I
don't understand why. Do you think it makes sense to consolidate it all in
VersionUpdaterWin? This seems good, since that's in the UI layer.

[bcwhite, 2015-07-02 03:11:49]

On 2015/07/01 18:26:00, grt wrote:
> On 2015/06/30 21:29:03, bcwhite wrote:
> > > Please do. It doesn't feel right to me to have html injected at this
layer.
> > > Thanks.
> > 
> > Taking a closer look...  The internationalization is occurring within
> > google_update, including the inclusion of the error code (and link).  If I
> move
> > link-injection to version_updater, I have to also move the i18n code, move
the
> > error code outside of the i18n string, or make the entire message a link. 
> What
> > do you think?
> 
> Wow, this is a mess. There's some string building in
> UpdateCheckDriver::OnUpgradeError and some in VersionUpdaterWin::OnError. I
> don't understand why. Do you think it makes sense to consolidate it all in
> VersionUpdaterWin? This seems good, since that's in the UI layer.

My assumption is that VersionUpdaterWin was to provide a base on top of which a
company-specific update mechanism could be implemented.  We have
google_update_win.cc but somebody else could build a different one for their own
purposes.  If so, I would say it was an unsuccessful separation since even
VersionUpdaterWin is checking for codes like GOOGLE_UPDATE_DISABLED_BY_POLICY.

VersionUpdaterWin is in the UI layer, but it's not Google specific.  My thoughts
are that it should provide only generic messages that can be overridden by
parameters (i.e. largely the way it is currently done except that the
GOOGLE_UPDATE_DISABLED_BY_POLICY case should be moved into the Google-specific
UpdateCheckDriver).

Injection of the link itself is, as you say, more a UI thing and
UpdateCheckDriver shouldn't assume that the passed "error_message" will be
displayed as HTML.  Three fixes for this that I can see:

1) Rename "error_message" to "html_error_message" so it's explicit instead of an
assumption.
2) Pass both "error_message" and "html_error_message" parameters.
3) Pass an additional "more_info_url" that OnError(...) would use to create a
separate "More Info" link rather than just turning the HRESULT code itself into
a link.

Personally I like #1 but think #3 is a reasonable alternative.  #2 just
complicates things to support a situation which will never occur in practice.

[grt (UTC plus 2), 2015-07-06 15:22:20]

On 2015/07/02 03:11:49, bcwhite wrote:
> On 2015/07/01 18:26:00, grt wrote:
> > On 2015/06/30 21:29:03, bcwhite wrote:
> > > > Please do. It doesn't feel right to me to have html injected at this
> layer.
> > > > Thanks.
> > > 
> > > Taking a closer look...  The internationalization is occurring within
> > > google_update, including the inclusion of the error code (and link).  If I
> > move
> > > link-injection to version_updater, I have to also move the i18n code, move
> the
> > > error code outside of the i18n string, or make the entire message a link. 
> > What
> > > do you think?
> > 
> > Wow, this is a mess. There's some string building in
> > UpdateCheckDriver::OnUpgradeError and some in VersionUpdaterWin::OnError. I
> > don't understand why. Do you think it makes sense to consolidate it all in
> > VersionUpdaterWin? This seems good, since that's in the UI layer.
> 
> My assumption is that VersionUpdaterWin was to provide a base on top of which
a
> company-specific update mechanism could be implemented.  We have
> google_update_win.cc but somebody else could build a different one for their
own
> purposes.  If so, I would say it was an unsuccessful separation since even
> VersionUpdaterWin is checking for codes like GOOGLE_UPDATE_DISABLED_BY_POLICY.
> 
> VersionUpdaterWin is in the UI layer, but it's not Google specific.  My
thoughts
> are that it should provide only generic messages that can be overridden by
> parameters (i.e. largely the way it is currently done except that the
> GOOGLE_UPDATE_DISABLED_BY_POLICY case should be moved into the Google-specific
> UpdateCheckDriver).
> 
> Injection of the link itself is, as you say, more a UI thing and
> UpdateCheckDriver shouldn't assume that the passed "error_message" will be
> displayed as HTML.  Three fixes for this that I can see:
> 
> 1) Rename "error_message" to "html_error_message" so it's explicit instead of
an
> assumption.
> 2) Pass both "error_message" and "html_error_message" parameters.
> 3) Pass an additional "more_info_url" that OnError(...) would use to create a
> separate "More Info" link rather than just turning the HRESULT code itself
into
> a link.
> 
> Personally I like #1 but think #3 is a reasonable alternative.  #2 just
> complicates things to support a situation which will never occur in practice.

I like the separation you describe. I think #1 with relevant updates to various
doc comments is okay since it keeps things pretty simple. If there's a need to
make the display of the strings more complicated, then we can complicate things
down here as needed.

[Will Harris, 2015-07-08 18:15:52]

On 2015/07/06 15:22:20, grt wrote:
> On 2015/07/02 03:11:49, bcwhite wrote:
> > On 2015/07/01 18:26:00, grt wrote:
> > > On 2015/06/30 21:29:03, bcwhite wrote:
> > > > > Please do. It doesn't feel right to me to have html injected at this
> > layer.
> > > > > Thanks.
> > > > 
> > > > Taking a closer look...  The internationalization is occurring within
> > > > google_update, including the inclusion of the error code (and link).  If
I
> > > move
> > > > link-injection to version_updater, I have to also move the i18n code,
move
> > the
> > > > error code outside of the i18n string, or make the entire message a
link. 
> > > What
> > > > do you think?
> > > 
> > > Wow, this is a mess. There's some string building in
> > > UpdateCheckDriver::OnUpgradeError and some in VersionUpdaterWin::OnError.
I
> > > don't understand why. Do you think it makes sense to consolidate it all in
> > > VersionUpdaterWin? This seems good, since that's in the UI layer.
> > 
> > My assumption is that VersionUpdaterWin was to provide a base on top of
which
> a
> > company-specific update mechanism could be implemented.  We have
> > google_update_win.cc but somebody else could build a different one for their
> own
> > purposes.  If so, I would say it was an unsuccessful separation since even
> > VersionUpdaterWin is checking for codes like
GOOGLE_UPDATE_DISABLED_BY_POLICY.
> > 
> > VersionUpdaterWin is in the UI layer, but it's not Google specific.  My
> thoughts
> > are that it should provide only generic messages that can be overridden by
> > parameters (i.e. largely the way it is currently done except that the
> > GOOGLE_UPDATE_DISABLED_BY_POLICY case should be moved into the
Google-specific
> > UpdateCheckDriver).
> > 
> > Injection of the link itself is, as you say, more a UI thing and
> > UpdateCheckDriver shouldn't assume that the passed "error_message" will be
> > displayed as HTML.  Three fixes for this that I can see:
> > 
> > 1) Rename "error_message" to "html_error_message" so it's explicit instead
of
> an
> > assumption.
> > 2) Pass both "error_message" and "html_error_message" parameters.
> > 3) Pass an additional "more_info_url" that OnError(...) would use to create
a
> > separate "More Info" link rather than just turning the HRESULT code itself
> into
> > a link.
> > 
> > Personally I like #1 but think #3 is a reasonable alternative.  #2 just
> > complicates things to support a situation which will never occur in
practice.
> 
> I like the separation you describe. I think #1 with relevant updates to
various
> doc comments is okay since it keeps things pretty simple. If there's a need to
> make the display of the strings more complicated, then we can complicate
things
> down here as needed.

this CL is causing a DCHECK as something is a non printable character

[4020:2316:0708/111225:FATAL:string_util_win.h(45)] Check failed:
IsWprintfFormatPortable(format). .???

32.1: kd:x86> k
ChildEBP          RetAddr           
05d0e268 752d0e6a base!base::debug::BreakDebugger+0x11
[d:\src\gclient\src\base\debug\debugger_win.cc @ 21]
05d0e6bc 75313a4a base!logging::LogMessage::~LogMessage+0x23a
[d:\src\gclient\src\base\logging.cc @ 642]
05d0e780 7531350f base!base::vswprintf+0x5a
[d:\src\gclient\src\base\strings\string_util_win.h @ 47]
(Inline) -------- base!base::?A0xf31e6877::vsnprintfT+0x18
[d:\src\gclient\src\base\strings\stringprintf.cc @ 37]
05d0f088 753139d7 base!base::`anonymous
namespace'::StringAppendVT<std::basic_string<wchar_t,std::char_traits<wchar_t>,std::allocator<wchar_t>
> >+0x5f [d:\src\gclient\src\base\strings\stringprintf.cc @ 58]
(Inline) -------- base!base::StringAppendV+0xd
[d:\src\gclient\src\base\strings\stringprintf.cc @ 182]
05d0f0a0 70179016 base!base::StringPrintf+0x27
[d:\src\gclient\src\base\strings\stringprintf.cc @ 127]
05d0f108 701775a2 chrome_6fdc0000!`anonymous
namespace'::UpdateCheckDriver::OnUpgradeError+0x96
[d:\src\gclient\src\chrome\browser\google\google_update_win.cc @ 726]
05d0f14c 752b129f chrome_6fdc0000!`anonymous
namespace'::UpdateCheckDriver::BeginUpdateCheck+0xd2
[d:\src\gclient\src\chrome\browser\google\google_update_win.cc @ 400]

[Will Harris, 2015-07-08 18:16:22]

wfh@chromium.org changed reviewers:
+ wfh@chromium.org

[Will Harris, 2015-07-08 18:16:34]

wfh@chromium.org changed reviewers:
- wfh@chromium.org

[bcwhite, 2015-07-08 19:18:18]

On 2015/07/08 18:15:52, Will Harris wrote:
> On 2015/07/06 15:22:20, grt wrote:
> > On 2015/07/02 03:11:49, bcwhite wrote:
> > > On 2015/07/01 18:26:00, grt wrote:
> > > > On 2015/06/30 21:29:03, bcwhite wrote:
> > > > > > Please do. It doesn't feel right to me to have html injected at this
> > > layer.
> > > > > > Thanks.
> > > > > 
> > > > > Taking a closer look...  The internationalization is occurring within
> > > > > google_update, including the inclusion of the error code (and link). 
If
> I
> > > > move
> > > > > link-injection to version_updater, I have to also move the i18n code,
> move
> > > the
> > > > > error code outside of the i18n string, or make the entire message a
> link. 
> > > > What
> > > > > do you think?
> > > > 
> > > > Wow, this is a mess. There's some string building in
> > > > UpdateCheckDriver::OnUpgradeError and some in
VersionUpdaterWin::OnError.
> I
> > > > don't understand why. Do you think it makes sense to consolidate it all
in
> > > > VersionUpdaterWin? This seems good, since that's in the UI layer.
> > > 
> > > My assumption is that VersionUpdaterWin was to provide a base on top of
> which
> > a
> > > company-specific update mechanism could be implemented.  We have
> > > google_update_win.cc but somebody else could build a different one for
their
> > own
> > > purposes.  If so, I would say it was an unsuccessful separation since even
> > > VersionUpdaterWin is checking for codes like
> GOOGLE_UPDATE_DISABLED_BY_POLICY.
> > > 
> > > VersionUpdaterWin is in the UI layer, but it's not Google specific.  My
> > thoughts
> > > are that it should provide only generic messages that can be overridden by
> > > parameters (i.e. largely the way it is currently done except that the
> > > GOOGLE_UPDATE_DISABLED_BY_POLICY case should be moved into the
> Google-specific
> > > UpdateCheckDriver).
> > > 
> > > Injection of the link itself is, as you say, more a UI thing and
> > > UpdateCheckDriver shouldn't assume that the passed "error_message" will be
> > > displayed as HTML.  Three fixes for this that I can see:
> > > 
> > > 1) Rename "error_message" to "html_error_message" so it's explicit instead
> of
> > an
> > > assumption.
> > > 2) Pass both "error_message" and "html_error_message" parameters.
> > > 3) Pass an additional "more_info_url" that OnError(...) would use to
create
> a
> > > separate "More Info" link rather than just turning the HRESULT code itself
> > into
> > > a link.
> > > 
> > > Personally I like #1 but think #3 is a reasonable alternative.  #2 just
> > > complicates things to support a situation which will never occur in
> practice.
> > 
> > I like the separation you describe. I think #1 with relevant updates to
> various
> > doc comments is okay since it keeps things pretty simple. If there's a need
to
> > make the display of the strings more complicated, then we can complicate
> things
> > down here as needed.
> 
> this CL is causing a DCHECK as something is a non printable character
> 
> [4020:2316:0708/111225:FATAL:string_util_win.h(45)] Check failed:
> IsWprintfFormatPortable(format). .???
> 
> 32.1: kd:x86> k
> ChildEBP          RetAddr           
> 05d0e268 752d0e6a base!base::debug::BreakDebugger+0x11
> [d:\src\gclient\src\base\debug\debugger_win.cc @ 21]
> 05d0e6bc 75313a4a base!logging::LogMessage::~LogMessage+0x23a
> [d:\src\gclient\src\base\logging.cc @ 642]
> 05d0e780 7531350f base!base::vswprintf+0x5a
> [d:\src\gclient\src\base\strings\string_util_win.h @ 47]
> (Inline) -------- base!base::?A0xf31e6877::vsnprintfT+0x18
> [d:\src\gclient\src\base\strings\stringprintf.cc @ 37]
> 05d0f088 753139d7 base!base::`anonymous
>
namespace'::StringAppendVT<std::basic_string<wchar_t,std::char_traits<wchar_t>,std::allocator<wchar_t>
> > >+0x5f [d:\src\gclient\src\base\strings\stringprintf.cc @ 58]
> (Inline) -------- base!base::StringAppendV+0xd
> [d:\src\gclient\src\base\strings\stringprintf.cc @ 182]
> 05d0f0a0 70179016 base!base::StringPrintf+0x27
> [d:\src\gclient\src\base\strings\stringprintf.cc @ 127]
> 05d0f108 701775a2 chrome_6fdc0000!`anonymous
> namespace'::UpdateCheckDriver::OnUpgradeError+0x96
> [d:\src\gclient\src\chrome\browser\google\google_update_win.cc @ 726]
> 05d0f14c 752b129f chrome_6fdc0000!`anonymous
> namespace'::UpdateCheckDriver::BeginUpdateCheck+0xd2
> [d:\src\gclient\src\chrome\browser\google\google_update_win.cc @ 400]

Looks like the "%s" needs to be "%ls".  Fixing coming soon...

[bcwhite, 2015-07-09 01:11:52]

Fix here: https://codereview.chromium.org/1221303021/