|
|
Created:
3 years, 7 months ago by Xiaocheng Modified:
3 years, 7 months ago Reviewers:
yosin_UTC9 CC:
blink-reviews, chromium-reviews Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionRefactor some first-letter handling code in TextIterator
This patch reorganizes code in TextIterator as a preparation to reduce
the level of hackiness in the class. The patch consists of:
1. Introduction of helper functions to reduce code duplication
2. Delay definition of |run_start| in HandleTextBox()
3. Changing some variable types of |const unsigned|
A follow-up patch will reduce the hackiness: crrev.com/2883163002
BUG=721957
TEST=n/a; no behavioral changes
Review-Url: https://codereview.chromium.org/2888463002
Cr-Commit-Position: refs/heads/master@{#472031}
Committed: https://chromium.googlesource.com/chromium/src/+/957d630f9df1f52475fa03759ea9451864b1a33b
Patch Set 1 #
Dependent Patchsets: Messages
Total messages: 28 (23 generated)
The CQ bit was checked by xiaochengh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Refactor some first-letter handling code in TextIterator BUG= ========== to ========== Refactor some first-letter handling code in TextIterator This patch reorganizes code in TextIterator as a preparation to reduce the level of hackiness in the class. The patch consists of: 1. Introduction of helper functions to reduce code duplication 2. 3. Changing some variable types of |const unsigned| BUG=721957 ==========
Description was changed from ========== Refactor some first-letter handling code in TextIterator This patch reorganizes code in TextIterator as a preparation to reduce the level of hackiness in the class. The patch consists of: 1. Introduction of helper functions to reduce code duplication 2. 3. Changing some variable types of |const unsigned| BUG=721957 ========== to ========== Refactor some first-letter handling code in TextIterator This patch reorganizes code in TextIterator as a preparation to reduce the level of hackiness in the class. The patch consists of: 1. Introduction of helper functions to reduce code duplication 2. Delay definition of |run_start| in HandleTextBox() 3. Changing some variable types of |const unsigned| BUG=721957 ==========
Description was changed from ========== Refactor some first-letter handling code in TextIterator This patch reorganizes code in TextIterator as a preparation to reduce the level of hackiness in the class. The patch consists of: 1. Introduction of helper functions to reduce code duplication 2. Delay definition of |run_start| in HandleTextBox() 3. Changing some variable types of |const unsigned| BUG=721957 ========== to ========== Refactor some first-letter handling code in TextIterator This patch reorganizes code in TextIterator as a preparation to reduce the level of hackiness in the class. The patch consists of: 1. Introduction of helper functions to reduce code duplication 2. Delay definition of |run_start| in HandleTextBox() 3. Changing some variable types of |const unsigned| A follow-up patch will reduce the hackiness: crrev.com/2883163002 BUG=721957 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
Description was changed from ========== Refactor some first-letter handling code in TextIterator This patch reorganizes code in TextIterator as a preparation to reduce the level of hackiness in the class. The patch consists of: 1. Introduction of helper functions to reduce code duplication 2. Delay definition of |run_start| in HandleTextBox() 3. Changing some variable types of |const unsigned| A follow-up patch will reduce the hackiness: crrev.com/2883163002 BUG=721957 ========== to ========== Refactor some first-letter handling code in TextIterator This patch reorganizes code in TextIterator as a preparation to reduce the level of hackiness in the class. The patch consists of: 1. Introduction of helper functions to reduce code duplication 2. Delay definition of |run_start| in HandleTextBox() 3. Changing some variable types of |const unsigned| A follow-up patch will reduce the hackiness: crrev.com/2883163002 BUG=721957 TEST=n/a; no behavioral changes ==========
The CQ bit was checked by xiaochengh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by xiaochengh@chromium.org to run a CQ dry run
xiaochengh@chromium.org changed reviewers: + yosin@chromium.org
PTAL.
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by xiaochengh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm BTW, for Layout NG adaptation, we have another approach, I prefer another way at this time, but it can be chagned. The another approach is Having TI::HandleNGText() and TI::HandleNGLineItem(), etc. instead of using current algorithm, == algorithm on LayoutText/InlineTextBox. When TI see NGLayoutBLockFlow, TI goes into another route. This approach needs to have more code, actually new code, but we can have: - clean implementation - No need for sorting inline box list - Utilize collected string - and more(?) I think it is worth to think. Note: Re-factoring of current implementation is good for understanding and current implementation.
The CQ bit was unchecked by yosin@chromium.org
The CQ bit was checked by yosin@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/05/16 at 04:28:24, yosin wrote: > lgtm > > BTW, for Layout NG adaptation, we have another approach, I prefer another way at this time, > but it can be chagned. > > The another approach is > > Having TI::HandleNGText() and TI::HandleNGLineItem(), etc. instead of using current algorithm, == algorithm on LayoutText/InlineTextBox. > > When TI see NGLayoutBLockFlow, TI goes into another route. > > This approach needs to have more code, actually new code, but we can have: > - clean implementation > - No need for sorting inline box list > - Utilize collected string > - and more(?) > > I think it is worth to think. > > Note: Re-factoring of current implementation is good for understanding and current implementation. They appear as the same approach to me. The main problem is to cleanup TI so that text box handling can be well encapsulated, so that we can easily switch to LayoutNG. The main problem is that the text box handling algorithm in TI is not very well encapsulated; some other components may use its saved states. For example, HandleReplacedElement checks the saved states when processing the last text node.
CQ is committing da patch. Bot data: {"patchset_id": 1, "attempt_start_ts": 1494908917281340, "parent_rev": "6fce1c41322e02c9c1d70bbe1c420ef8269896de", "commit_rev": "957d630f9df1f52475fa03759ea9451864b1a33b"}
Message was sent while issue was closed.
Description was changed from ========== Refactor some first-letter handling code in TextIterator This patch reorganizes code in TextIterator as a preparation to reduce the level of hackiness in the class. The patch consists of: 1. Introduction of helper functions to reduce code duplication 2. Delay definition of |run_start| in HandleTextBox() 3. Changing some variable types of |const unsigned| A follow-up patch will reduce the hackiness: crrev.com/2883163002 BUG=721957 TEST=n/a; no behavioral changes ========== to ========== Refactor some first-letter handling code in TextIterator This patch reorganizes code in TextIterator as a preparation to reduce the level of hackiness in the class. The patch consists of: 1. Introduction of helper functions to reduce code duplication 2. Delay definition of |run_start| in HandleTextBox() 3. Changing some variable types of |const unsigned| A follow-up patch will reduce the hackiness: crrev.com/2883163002 BUG=721957 TEST=n/a; no behavioral changes Review-Url: https://codereview.chromium.org/2888463002 Cr-Commit-Position: refs/heads/master@{#472031} Committed: https://chromium.googlesource.com/chromium/src/+/957d630f9df1f52475fa03759ea9... ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1) as https://chromium.googlesource.com/chromium/src/+/957d630f9df1f52475fa03759ea9... |