force constrained signed flow in avatar bubble on mac

force constrained signed flow in avatar bubble on mac

BUG=341634
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=255519

[guohui, 2014-03-06 18:01:23]

Hey,

could you please take a look at the CL?

Thanks,

Hui

[Alexei Svitkine (slow), 2014-03-06 18:08:24]

This change is okay, I guess - LGTM.

However, the API can be improved:
  - Overloading is highly discourage in Chromium, so I suggest migrating
everything to the 3-param version and removing the 2-param version. (Maybe
that's what you're in the process of doing here.)
  - It's better to change the bool params to enums so that it's more obvious
what's being specified at the call site (without needing to add comments like
you've done here). This also makes it so the compiler will catch if you get the
order of params wrong.

It would be good to refactor the API with the above suggestions, but this can be
done in another CL.

[guohui, 2014-03-06 18:34:52]

On 2014/03/06 18:08:24, Alexei Svitkine wrote:
> This change is okay, I guess - LGTM.
> 
> However, the API can be improved:
>   - Overloading is highly discourage in Chromium, so I suggest migrating
> everything to the 3-param version and removing the 2-param version. (Maybe
> that's what you're in the process of doing here.)
>   - It's better to change the bool params to enums so that it's more obvious
> what's being specified at the call site (without needing to add comments like
> you've done here). This also makes it so the compiler will catch if you get
the
> order of params wrong.
> 
> It would be good to refactor the API with the above suggestions, but this can
be
> done in another CL.

agree. Filed crbug/349990.

[guohui, 2014-03-06 18:34:55]

The CQ bit was checked by guohui@chromium.org

[commit-bot: I haz the power, 2014-03-06 18:40:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/guohui@chromium.org/188753003/1

[commit-bot: I haz the power, 2014-03-06 18:50:39]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-06 18:50:39]

Try jobs failed on following builders: mac_chromium_compile_dbg

[guohui, 2014-03-06 20:13:47]

The CQ bit was checked by guohui@chromium.org

[commit-bot: I haz the power, 2014-03-06 20:56:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/guohui@chromium.org/188753003/1

[commit-bot: I haz the power, 2014-03-06 21:44:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/guohui@chromium.org/188753003/1

[commit-bot: I haz the power, 2014-03-06 22:19:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/guohui@chromium.org/188753003/1

[commit-bot: I haz the power, 2014-03-07 04:04:52]

Change committed as 255519