Always initialize |m_totalWidth| in HarfBuzzShaper::shape.

Always initialize |m_totalWidth| in HarfBuzzShaper::shape.

R=leviw@chromium.org
BUG=476647
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=194541

[Martin Barbella, 2015-04-24 23:08:25]

It would be nice to add the test for this as well, but I'm realizing that I
don't know enough about how LayoutTests/ is structured to know where to put it.

Maybe fast/text/shaping?

[leviw_travelin_and_unemployed, 2015-04-24 23:37:04]

On 2015/04/24 at 23:08:25, mbarbella wrote:
> It would be nice to add the test for this as well, but I'm realizing that I
don't know enough about how LayoutTests/ is structured to know where to put it.
> 
> Maybe fast/text/shaping?

We definitely need a test. fast/text/shaping is a fine place.

It's fine if your test only works on ASAN. I'm going to want the test actually
*minimized*, though, just to make it clearer how we got into this mess. That
part tends to suck, FYI.

[Martin Barbella, 2015-04-27 05:37:52]

On 2015/04/24 23:37:04, leviw wrote:
> On 2015/04/24 at 23:08:25, mbarbella wrote:
> > It would be nice to add the test for this as well, but I'm realizing that I
> don't know enough about how LayoutTests/ is structured to know where to put
it.
> > 
> > Maybe fast/text/shaping?
> 
> We definitely need a test. fast/text/shaping is a fine place.
> 
> It's fine if your test only works on ASAN. I'm going to want the test actually
> *minimized*, though, just to make it clearer how we got into this mess. That
> part tends to suck, FYI.

Added the test. Going through some of the docs, I'm having trouble figuring out
what I need to do next to get this to pass on the bots.

On a side note, I'm always interested in ClusterFuzz reports that aren't
minimizing well since I recently rewrote its minimizer. In this particular case,
the fuzzer does a few things that will have to be manually minimized out, but
I'm pretty sure we could have automated a bit more of it. I'll take a closer
look this week.

[leviw_travelin_and_unemployed, 2015-04-27 05:49:57]

On 2015/04/27 at 05:37:52, mbarbella wrote:
> On 2015/04/24 23:37:04, leviw wrote:
> > On 2015/04/24 at 23:08:25, mbarbella wrote:
> > > It would be nice to add the test for this as well, but I'm realizing that
I
> > don't know enough about how LayoutTests/ is structured to know where to put
it.
> > > 
> > > Maybe fast/text/shaping?
> > 
> > We definitely need a test. fast/text/shaping is a fine place.
> > 
> > It's fine if your test only works on ASAN. I'm going to want the test
actually
> > *minimized*, though, just to make it clearer how we got into this mess. That
> > part tends to suck, FYI.
> 
> Added the test. Going through some of the docs, I'm having trouble figuring
out what I need to do next to get this to pass on the bots.

You need test expectations. First add this:

if (window.testRunner)
  testRunner.dumpAsText();

Then run the test (run-webkit-tests
fast/text/shaping/shaping-width-initialized.html)

Then add the fast/text/shaping/shaping-width-initialized-expected.txt file and
re-upload :)

> 
> On a side note, I'm always interested in ClusterFuzz reports that aren't
minimizing well since I recently rewrote its minimizer. In this particular case,
the fuzzer does a few things that will have to be manually minimized out, but
I'm pretty sure we could have automated a bit more of it. I'll take a closer
look this week.

https://www.youtube.com/watch?v=ItWIYcD6jas

It's already so much better than before. Thanks for caring about that.
Clusterfuzz test minimization seemed like my middle-name for awhile.

[Martin Barbella, 2015-04-27 15:21:35]

On 2015/04/27 05:49:57, leviw wrote:
> On 2015/04/27 at 05:37:52, mbarbella wrote:
> > On 2015/04/24 23:37:04, leviw wrote:
> > > On 2015/04/24 at 23:08:25, mbarbella wrote:
> > > > It would be nice to add the test for this as well, but I'm realizing
that
> I
> > > don't know enough about how LayoutTests/ is structured to know where to
put
> it.
> > > > 
> > > > Maybe fast/text/shaping?
> > > 
> > > We definitely need a test. fast/text/shaping is a fine place.
> > > 
> > > It's fine if your test only works on ASAN. I'm going to want the test
> actually
> > > *minimized*, though, just to make it clearer how we got into this mess.
That
> > > part tends to suck, FYI.
> > 
> > Added the test. Going through some of the docs, I'm having trouble figuring
> out what I need to do next to get this to pass on the bots.
> 
> You need test expectations. First add this:
> 
> if (window.testRunner)
>   testRunner.dumpAsText();
> 
> Then run the test (run-webkit-tests
> fast/text/shaping/shaping-width-initialized.html)
> 
> Then add the fast/text/shaping/shaping-width-initialized-expected.txt file and
> re-upload :)
> 
> > 
> > On a side note, I'm always interested in ClusterFuzz reports that aren't
> minimizing well since I recently rewrote its minimizer. In this particular
case,
> the fuzzer does a few things that will have to be manually minimized out, but
> I'm pretty sure we could have automated a bit more of it. I'll take a closer
> look this week.
> 
> https://www.youtube.com/watch?v=ItWIYcD6jas
> 
> It's already so much better than before. Thanks for caring about that.
> Clusterfuzz test minimization seemed like my middle-name for awhile.

Done. Thanks for all of the quick replies.

[Daniel Erat, 2015-04-27 15:42:21]

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

[Daniel Erat, 2015-04-27 15:42:21]

https://codereview.chromium.org/1108663003/diff/40001/Source/platform/fonts/s...
File Source/platform/fonts/shaping/HarfBuzzShaper.cpp (right):

https://codereview.chromium.org/1108663003/diff/40001/Source/platform/fonts/s...
Source/platform/fonts/shaping/HarfBuzzShaper.cpp:541: m_totalWidth = 0;
mind initializing it in the c'tor as well? having uninitialized members hanging
around makes me nervous.

[Martin Barbella, 2015-04-27 15:51:30]

On 2015/04/27 15:42:21, Daniel Erat wrote:
>
https://codereview.chromium.org/1108663003/diff/40001/Source/platform/fonts/s...
> File Source/platform/fonts/shaping/HarfBuzzShaper.cpp (right):
> 
>
https://codereview.chromium.org/1108663003/diff/40001/Source/platform/fonts/s...
> Source/platform/fonts/shaping/HarfBuzzShaper.cpp:541: m_totalWidth = 0;
> mind initializing it in the c'tor as well? having uninitialized members
hanging
> around makes me nervous.

Done.

[Daniel Erat, 2015-04-27 15:53:02]

thanks, lgtm

[leviw_travelin_and_unemployed, 2015-04-27 18:19:35]

https://codereview.chromium.org/1108663003/diff/60001/Source/platform/fonts/s...
File Source/platform/fonts/shaping/HarfBuzzShaper.cpp (right):

https://codereview.chromium.org/1108663003/diff/60001/Source/platform/fonts/s...
Source/platform/fonts/shaping/HarfBuzzShaper.cpp:542: m_totalWidth = 0;
This should be redundant now that it's in the c'tor, no?

[Martin Barbella, 2015-04-27 18:25:14]

On 2015/04/27 18:19:35, leviw wrote:
>
https://codereview.chromium.org/1108663003/diff/60001/Source/platform/fonts/s...
> File Source/platform/fonts/shaping/HarfBuzzShaper.cpp (right):
> 
>
https://codereview.chromium.org/1108663003/diff/60001/Source/platform/fonts/s...
> Source/platform/fonts/shaping/HarfBuzzShaper.cpp:542: m_totalWidth = 0;
> This should be redundant now that it's in the c'tor, no?

Yep, seems like it. Updated.

I wasn't sure if there were any cases where shape was called twice, but it seems
like there aren't.

[leviw_travelin_and_unemployed, 2015-04-27 18:27:38]

The CQ bit was checked by leviw@chromium.org

[leviw_travelin_and_unemployed, 2015-04-27 18:27:39]

lgtm

[leviw_travelin_and_unemployed, 2015-04-27 18:27:39]

The patchset sent to the CQ was uploaded after l-g-t-m from derat@chromium.org
Link to the patchset: https://codereview.chromium.org/1108663003/#ps80001
(title: "Remove redundant initialization")

[commit-bot: I haz the power, 2015-04-27 18:28:05]

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

[commit-bot: I haz the power, 2015-04-27 19:37:51]

Committed patchset #5 (id:80001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=194541