New issue
Advanced search Search tips

Issue 616967 link

Starred by 1 user
Status: Available
Owner: ----
Cc:
qyears...@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Feature



Sign in to add a comment

Reorganize rebaseline commands so that there's one command per module.

Project Member Reported by qyears...@chromium.org, Jun 3 2016 
Currently all of the rebasing-related commands are in one file, webkitpy/tool/commands/rebaseline.py. Current class tree:

Command
    AbstractRebaseliningCommand
    BaseInternalRebaselineCommand
        CopyExistingBaselinesInternal
        RebaselineTest
    AbstractRebaseliningCommand
    AnalyzeBaselines
    AbstractParallelRebaselineCommand
        RebaselineJson
        RebaselineExpectations
        Rebaseline
        AutoRebaseline

It maybe easier to navigate the code if there's one class per file, and if the files have slightly clearer names. Proposal:

1. Merge BaseInternalRebaselineCommand and AbstractRebaseliningCommand to make a base class (BaseInternalRebaselineCommand doesn't contain much now) called AbstractRebaselineCommand.

2. Rename some classes. Possible renaming:
      RebaselineTest -> RebaselineSingleTest
      CopyExistingBaselinesInternal -> CopyExistingBaselines
      Rebaseline -> RebaselineMultipleTests?

3. Split classes into separate files, e.g.:

  abstract_rebaseline_command.py
  rebaseline_single_test.py
  analyze_baselines.py
  optimize_baselines.py
  ...

Also, somewhat relatedly, maybe AnalyzeBaselines, OptimizeBaselines, and CopyExistingBaselines might not need to be subclasses of AbstractRebaselineCommand.

Comment 4 by qyears...@chromium.org, Aug 24 2016

Labels: -OS-Linux -Type-Bug Type-Feature
Status: Started (was: Assigned)
Summary: Reorganize rebaseline commands so that there's one command per module. (was: Reorganize rebaseline commands.)

Comment 5 by qyears...@chromium.org, Sep 21 2016

Status: Assigned (was: Started)

Comment 6 by qyears...@chromium.org, Oct 21 2016

Labels: Hotlist-CodeHealth
Project Member

Comment 7 by bugdroid1@chromium.org, Oct 27 2016 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/3078bc85c2b7d7f8feddd8d19b7a11e2fbed9d90

commit 3078bc85c2b7d7f8feddd8d19b7a11e2fbed9d90
Author: qyearsley <qyearsley@chromium.org>
Date: Thu Oct 27 22:14:09 2016

Refactoring in rebaseline.py: Combine BaseInternalRebaselineCommand and AbstractRebaseliningCommand.

Reason: this simplifies the class hierarchy.

BUG=616967

Review-Url: https://codereview.chromium.org/2451243002
Cr-Commit-Position: refs/heads/master@{#428159}

[modify] https://crrev.com/3078bc85c2b7d7f8feddd8d19b7a11e2fbed9d90/third_party/WebKit/Tools/Scripts/webkitpy/tool/commands/rebaseline.py

Comment 8 by qyears...@chromium.org, Apr 6 2017

Status: Started (was: Assigned)
Update: I'm still planning to do this, although it's still low-priority.
Project Member

Comment 9 by bugdroid1@chromium.org, Jun 1 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/4b31acdd37eb70ad84fbd68a03ee2e66acb93f99

commit 4b31acdd37eb70ad84fbd68a03ee2e66acb93f99
Author: Quinten Yearsley <qyearsley@google.com>
Date: Thu Jun 01 22:51:22 2017

Extract "internal" rebaseline commands to separate files.

This is a continuation of changes in http://crbug.com/616967; the purpose
is to make it so that in general there's one class per file, and files
aren't too large.

This CL also makes a couple of minor clean-up changes in the classes
that are moved, e.g. changing method order, but this CL doesn't
include any changes to behavior.

Bug: 616967
Change-Id: I4f6aade879d0f660d3f35668448028e8ef0651b0
Reviewed-on: https://chromium-review.googlesource.com/521787
Commit-Queue: Quinten Yearsley <qyearsley@chromium.org>
Reviewed-by: Xianzhu Wang <wangxianzhu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#476465}
[add] https://crrev.com/4b31acdd37eb70ad84fbd68a03ee2e66acb93f99/third_party/WebKit/Tools/Scripts/webkitpy/tool/commands/copy_existing_baselines.py
[add] https://crrev.com/4b31acdd37eb70ad84fbd68a03ee2e66acb93f99/third_party/WebKit/Tools/Scripts/webkitpy/tool/commands/copy_existing_baselines_unittest.py
[modify] https://crrev.com/4b31acdd37eb70ad84fbd68a03ee2e66acb93f99/third_party/WebKit/Tools/Scripts/webkitpy/tool/commands/rebaseline.py
[add] https://crrev.com/4b31acdd37eb70ad84fbd68a03ee2e66acb93f99/third_party/WebKit/Tools/Scripts/webkitpy/tool/commands/rebaseline_test.py
[add] https://crrev.com/4b31acdd37eb70ad84fbd68a03ee2e66acb93f99/third_party/WebKit/Tools/Scripts/webkitpy/tool/commands/rebaseline_test_unittest.py
[modify] https://crrev.com/4b31acdd37eb70ad84fbd68a03ee2e66acb93f99/third_party/WebKit/Tools/Scripts/webkitpy/tool/commands/rebaseline_unittest.py
[modify] https://crrev.com/4b31acdd37eb70ad84fbd68a03ee2e66acb93f99/third_party/WebKit/Tools/Scripts/webkitpy/tool/webkit_patch.py

Comment 10 by qyears...@chromium.org, Jul 20 2017 

Tasks still left here:

Extract the following classes out of rebaseline.py:

    AbstractRebaseliningCommand
    AbstractParallelRebaselineCommand
    RebaselineExpectations
    TestBaselineSet

Related refactoring task: get rid of ChangeSet.

Comment 11 by qyears...@chromium.org, Aug 23 2017

Cc: qyears...@chromium.org
Owner: ----
Status: Available (was: Started)
Project Member

Comment 12 by sheriffbot@chromium.org, Aug 24

Labels: Hotlist-Recharge-Cold
Status: Untriaged (was: Available)
This issue has been Available for over a year. If it's no longer important or seems unlikely to be fixed, please consider closing it out. If it is important, please re-triage the issue.

Sorry for the inconvenience if the bug really should have been left as Available.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Comment 13 by foolip@chromium.org, Sep 24

Status: Available (was: Untriaged)
Remaining work described in #10.
►

Sign in to add a comment