Add TODO for confusing code in layout_test_runner_unittest.py.

Add TODO for confusing code in layout_test_runner_unittest.py.

Review-Url: https://codereview.chromium.org/2604223002
Cr-Commit-Position: refs/heads/master@{#443144}
Committed: https://chromium.googlesource.com/chromium/src/+/4e689e4b3bf393feb419ee1d2c6b6622163f0f55

[qyearsley, 2017-01-10 21:11:55]

qyearsley@chromium.org changed reviewers:
+ jeffcarp@chromium.org

[qyearsley, 2017-01-10 21:11:55]

I made this CL a little while ago while reading through some apparently-unused
functions (for http://crbug.com/677561); do you think it this makes the code
clearer?

Note, I'm not sure when I would come back and revisit this.

[jeffcarp, 2017-01-10 22:18:41]

On 2017/01/10 at 21:11:55, qyearsley wrote:
> I made this CL a little while ago while reading through some apparently-unused
functions (for http://crbug.com/677561); do you think it this makes the code
clearer?
> 
> Note, I'm not sure when I would come back and revisit this.

I agree it makes the code clearer. lgtm. More thoughts:

- I think it would make the code even more clearer by removing the
metaprogramming in handle() (maybe this is what you were suggesting?). I also
understand it's not super high priority. We could make a separate bug for it?
- It brings up the question: when should things be TODOs in code and when should
they be bugs? Or both?
- If we can prove it's not called anywhere, I vote that we just delete the
function outright.

[qyearsley, 2017-01-11 00:05:01]

On 2017/01/10 at 22:18:41, jeffcarp wrote:
> On 2017/01/10 at 21:11:55, qyearsley wrote:
> > I made this CL a little while ago while reading through some
apparently-unused functions (for http://crbug.com/677561); do you think it this
makes the code clearer?
> > 
> > Note, I'm not sure when I would come back and revisit this.
> 
> I agree it makes the code clearer. lgtm. More thoughts:
> 
> - I think it would make the code even more clearer by removing the
metaprogramming in handle() (maybe this is what you were suggesting?). I also
understand it's not super high priority. We could make a separate bug for it?
> - It brings up the question: when should things be TODOs in code and when
should they be bugs? Or both?

Yeah - Here's what I think:

 - TODOs in the code should be used when there's some small task which can be
done in one follow-up CL and doesn't require discussion.
 - Bugs/issues should be filed when discussion or multiple CLs are required.
 - There's usually no need to have both a bug and and a TODO for the same task,
but there's nothing wrong with that either.
 - Sometimes TODOs reference bugs, e.g. "TODO(contact): Remove this when
crbug.com/xxx is fixed."

> - If we can prove it's not called anywhere, I vote that we just delete the
function outright.

Yeah -- the reason why I didn't want to do that is because if that method is
removed, then I don't see the point of LockCheckingRunner at all; I feel like
there's probably something I'm missing or not understanding.

[qyearsley, 2017-01-11 00:05:06]

The CQ bit was checked by qyearsley@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-11 00:05:39]

Dry run: 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, 2017-01-11 01:21:15]

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

[commit-bot: I haz the power, 2017-01-11 01:21:15]

Dry run: This issue passed the CQ dry run.

[qyearsley, 2017-01-12 02:33:17]

The CQ bit was checked by qyearsley@chromium.org

[commit-bot: I haz the power, 2017-01-12 02:33:39]

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, 2017-01-12 03:56:59]

CQ is committing da patch.

Bot data: {"patchset_id": 1, "attempt_start_ts": 1484188397324030, "parent_rev":
"f42add702e5cbf42601dd53e27456ca0ec747c27", "commit_rev":
"4e689e4b3bf393feb419ee1d2c6b6622163f0f55"}

[commit-bot: I haz the power, 2017-01-12 03:57:36]

Description was changed from

==========
Add TODO for confusing code in layout_test_runner_unittest.py.
==========

to

==========
Add TODO for confusing code in layout_test_runner_unittest.py.

Review-Url: https://codereview.chromium.org/2604223002
Cr-Commit-Position: refs/heads/master@{#443144}
Committed:
https://chromium.googlesource.com/chromium/src/+/4e689e4b3bf393feb419ee1d2c6b...
==========

[commit-bot: I haz the power, 2017-01-12 03:57:36]

Committed patchset #1 (id:1) as
https://chromium.googlesource.com/chromium/src/+/4e689e4b3bf393feb419ee1d2c6b...