Alt text for About UI product label

Alt text for About UI product label

BUG=427109
TBR=dbeam@chromium.org,stevenjb@chromium.org

See http://crrev.com/677923002

Committed: https://crrev.com/3b0ecf38dcf5babb189952d88c188af3e7b86329
Cr-Commit-Position: refs/heads/master@{#301711}

[michaelpg, 2014-10-28 17:49:53]

The CQ bit was checked by michaelpg@chromium.org

[commit-bot: I haz the power, 2014-10-28 17:51:07]

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

[commit-bot: I haz the power, 2014-10-28 21:17:07]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2014-10-28 21:17:47]

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

[satorux1, 2014-12-04 08:20:03]

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

[satorux1, 2014-12-04 08:20:04]

Hmm... I wonder why this patch was submitted with a TBR. The code had bugs which
could have been caught by a code review. Please don't do this again.

https://codereview.chromium.org/680393002/diff/1/chrome/browser/ui/webui/help...
File chrome/browser/ui/webui/help/help_handler.cc (right):

https://codereview.chromium.org/680393002/diff/1/chrome/browser/ui/webui/help...
chrome/browser/ui/webui/help/help_handler.cc:395:
base::Bind(&HelpHandler::LoadFCCLabelText, weak_factory_.GetWeakPtr()));
This code is incorrect. You cannot use a UI-thread originated WeakPtr to post a
task to FILE thread.

https://codereview.chromium.org/680393002/diff/1/chrome/browser/ui/webui/help...
chrome/browser/ui/webui/help/help_handler.cc:584:
web_ui()->CallJavascriptFunction("help.HelpPage.setProductLabelText",
This code is probably incorrect too. It looks unsafe to call
web_ui()->CallJavascriptFunction() on FILE thread.

The right thing to do would be to use PostTaskAndReplyWithResult() to read the
file on a worker pool thread, and call the CallJavascriptFunction() on the UI
thread.

[satorux1, 2014-12-04 08:27:58]

Filed a bug: crbug.com/438953

[stevenjb, 2014-12-04 18:28:07]

On 2014/12/04 08:20:04, satorux1 wrote:
> Hmm... I wonder why this patch was submitted with a TBR. The code had bugs
which
> could have been caught by a code review. Please don't do this again.
> 
>
https://codereview.chromium.org/680393002/diff/1/chrome/browser/ui/webui/help...
> File chrome/browser/ui/webui/help/help_handler.cc (right):
> 
>
https://codereview.chromium.org/680393002/diff/1/chrome/browser/ui/webui/help...
> chrome/browser/ui/webui/help/help_handler.cc:395:
> base::Bind(&HelpHandler::LoadFCCLabelText, weak_factory_.GetWeakPtr()));
> This code is incorrect. You cannot use a UI-thread originated WeakPtr to post
a
> task to FILE thread.
> 
>
https://codereview.chromium.org/680393002/diff/1/chrome/browser/ui/webui/help...
> chrome/browser/ui/webui/help/help_handler.cc:584:
> web_ui()->CallJavascriptFunction("help.HelpPage.setProductLabelText",
> This code is probably incorrect too. It looks unsafe to call
> web_ui()->CallJavascriptFunction() on FILE thread.
> 
> The right thing to do would be to use PostTaskAndReplyWithResult() to read the
> file on a worker pool thread, and call the CallJavascriptFunction() on the UI
> thread.

This is my bad. As mentioned in the description, this was originally reviewed in
http://crrev.com/677923002 and I missed this.

[satorux1, 2014-12-04 22:01:17]

> This is my bad. As mentioned in the description, this was originally reviewed
in
> http://crrev.com/677923002 and I missed this.

Thanks. Didn't notice the link. That explains.

[stevenjb, 2014-12-04 23:53:31]

No worries. Thanks for catching that, and for filing a bug to fix it!


On Thu, Dec 4, 2014 at 2:01 PM, <satorux@chromium.org> wrote:

> This is my bad. As mentioned in the description, this was originally
>> reviewed
>>
> in
>
>> http://crrev.com/677923002 and I missed this.
>>
>
> Thanks. Didn't notice the link. That explains.
>
> https://codereview.chromium.org/680393002/
>

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