Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2800133003:
Avoid duplicate functions: one AddStringToDigestor is enough (Closed)
|
Created:
3 years, 8 months ago by Daniel Bratell Modified:
3 years, 5 months ago Reviewers:
alph CC:
chromium-reviews, caseq+blink_chromium.org, shans, rjwright, blink-reviews-animation_chromium.org, darktears, lushnikov+blink_chromium.org, pfeldman+blink_chromium.org, apavlov+blink_chromium.org, devtools-reviews_chromium.org, blink-reviews, Eric Willigers, kozyatinskiy+blink_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionAvoid duplicate functions: one AddStringToDigestor is enough
There are two implementation of AddStringToDigestor. Beyond being
one more than needed, they also conflict in jumbo builds.
This patch merges the two AddStringToDigestor and removes the
conflicting files from the jumbo exclusion list.
Review-Url: https://codereview.chromium.org/2800133003
Cr-Commit-Position: refs/heads/master@{#485992}
Committed: https://chromium.googlesource.com/chromium/src/+/0ba26ac61aba0f76c0c3d948126264af5b43cd8b
Patch Set 1 #
Total comments: 2
Patch Set 2 : fixup core/inspector license #Patch Set 3 : Rebased past blink rename #Patch Set 4 : Avoid duplicate functions/code: one AddStringToDigestor is enough. #
Total comments: 5
Patch Set 5 : New idea. Include the string type in the name. #
Total comments: 2
Patch Set 6 : Use utf-8 and get all data from the same object #
Total comments: 4
Patch Set 7 : Adding spacer lines #
Messages
Total messages: 46 (30 generated)
bratell@opera.com changed reviewers: + alph@chromium.org
Please take a look.
The CQ bit was checked by bratell@opera.com 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: This issue passed the CQ dry run.
https://codereview.chromium.org/2800133003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/inspector/addStringToDigestor.cpp (right): https://codereview.chromium.org/2800133003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/inspector/addStringToDigestor.cpp:2: // Use of this source code is governed by a BSD-style license that can be missing a line of copyright
Doh, indeed. New version with more lines uploaded. https://codereview.chromium.org/2800133003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/inspector/addStringToDigestor.cpp (right): https://codereview.chromium.org/2800133003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/inspector/addStringToDigestor.cpp:2: // Use of this source code is governed by a BSD-style license that can be On 2017/04/10 08:09:55, alph_ooo wrote: > missing a line of copyright Done.
On 2017/04/10 08:28:29, Daniel Bratell wrote: > Doh, indeed. New version with more lines uploaded. > > https://codereview.chromium.org/2800133003/diff/1/third_party/WebKit/Source/c... > File third_party/WebKit/Source/core/inspector/addStringToDigestor.cpp (right): > > https://codereview.chromium.org/2800133003/diff/1/third_party/WebKit/Source/c... > third_party/WebKit/Source/core/inspector/addStringToDigestor.cpp:2: // Use of > this source code is governed by a BSD-style license that can be > On 2017/04/10 08:09:55, alph_ooo wrote: > > missing a line of copyright > > Done. But maybe you have a suggestion for a better location for the function? I found nothing obvious.
The CQ bit was checked by bratell@opera.com 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: This issue passed the CQ dry run.
On 2017/04/10 08:29:39, Daniel Bratell wrote: > On 2017/04/10 08:28:29, Daniel Bratell wrote: > > Doh, indeed. New version with more lines uploaded. > > > > > https://codereview.chromium.org/2800133003/diff/1/third_party/WebKit/Source/c... > > File third_party/WebKit/Source/core/inspector/addStringToDigestor.cpp (right): > > > > > https://codereview.chromium.org/2800133003/diff/1/third_party/WebKit/Source/c... > > third_party/WebKit/Source/core/inspector/addStringToDigestor.cpp:2: // Use of > > this source code is governed by a BSD-style license that can be > > On 2017/04/10 08:09:55, alph_ooo wrote: > > > missing a line of copyright > > > > Done. > > But maybe you have a suggestion for a better location for the function? I found > nothing obvious. What's the current status of this CL? Let's not proceed with it per pfeldman comments in https://codereview.chromium.org/2800213002/
Description was changed from ========== Avoid duplicate functions/code in core/inspector: addStringToDigestor While experimenting with unity builds I encountered a few duplicate symbols and functions in core/inspector. One of them was addStringToDigestor which is a function that identically defined at two places. This patch moves that function to a shared file and uses it from there. ========== to ========== Avoid duplicate functions/code in core/inspector: addStringToDigestor While experimenting with unity builds I encountered a few duplicate symbols and functions in core/inspector. One of them was addStringToDigestor which is a function that identically defined at two places. This patch moves that function to a shared file and uses it from there. ==========
alph@chromium.org changed reviewers: - alph@chromium.org
On 2017/05/02 23:50:43, alph wrote: > What's the current status of this CL? > Let's not proceed with it per pfeldman comments in > https://codereview.chromium.org/2800213002/ I realized that I hadn't updated you on the progress, and only a small part of it has been visible in blink-dev. Jumbo builds are looking increasingly promising and I hope that will move ahead once people are back. But I think this should be treated primarily as a patch to deduplicate code. Having more than one implementation of a function is not optimal I think, but I am open to alternative ways to make it one function instead of two. You know the code better and might have some suggestions.
The CQ bit was checked by bratell@opera.com 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...
alph, can you take a new look please?
Description was changed from ========== Avoid duplicate functions/code in core/inspector: addStringToDigestor While experimenting with unity builds I encountered a few duplicate symbols and functions in core/inspector. One of them was addStringToDigestor which is a function that identically defined at two places. This patch moves that function to a shared file and uses it from there. ========== to ========== Avoid duplicate functions: one AddStringToDigestor is enough There are two implementation of AddStringToDigestor. Beyond being one more than needed, they also conflict in jumbo builds. This patch merges the two AddStringToDigestor and removes the conflicting files from the jumbo exclusion list. ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
bratell@opera.com changed reviewers: + alph@chromium.org
alph, can you take a look please?
Sorry for delay, was on vacation. :-) https://codereview.chromium.org/2800133003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/inspector/AddStringToDigestor.cpp (right): https://codereview.chromium.org/2800133003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/AddStringToDigestor.cpp:14: reinterpret_cast<const unsigned char*>(string.Ascii().data()), I bet string.Ascii().length() is not equal to string.length() Also let's use Utf8() instead. https://codereview.chromium.org/2800133003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/inspector/AddStringToDigestor.h (right): https://codereview.chromium.org/2800133003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/AddStringToDigestor.h:16: #endif // AddStringToDigestor_h nit add an empty string above
The CQ bit was checked by bratell@opera.com 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...
alph, I made another solution since the functions were not identical but let me know if you rather merge them and let the ascii version be converted to the utf8 version. https://codereview.chromium.org/2800133003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/inspector/AddStringToDigestor.cpp (right): https://codereview.chromium.org/2800133003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/AddStringToDigestor.cpp:14: reinterpret_cast<const unsigned char*>(string.Ascii().data()), On 2017/07/08 01:16:24, alph wrote: > I bet string.Ascii().length() is not equal to string.length() > Also let's use Utf8() instead. So thanks to your review I realized that the two functions are not identical. One does ASCII and one does UTF-8. So merging them will mean a change. I did a new patch which keeps the old behaviour, but if you prefer this, I can switch to utf-8 instead. https://codereview.chromium.org/2800133003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/inspector/AddStringToDigestor.h (right): https://codereview.chromium.org/2800133003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/AddStringToDigestor.h:16: #endif // AddStringToDigestor_h On 2017/07/08 01:16:24, alph wrote: > nit add an empty string above Dropping this file.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Don't you want to make the change to BUILD.gn ? https://codereview.chromium.org/2800133003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/inspector/AddStringToDigestor.cpp (right): https://codereview.chromium.org/2800133003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/AddStringToDigestor.cpp:14: reinterpret_cast<const unsigned char*>(string.Ascii().data()), On 2017/07/10 14:47:18, Daniel Bratell wrote: > On 2017/07/08 01:16:24, alph wrote: > > I bet string.Ascii().length() is not equal to string.length() > > Also let's use Utf8() instead. > > So thanks to your review I realized that the two functions are not identical. > One does ASCII and one does UTF-8. So merging them will mean a change. I did a > new patch which keeps the old behaviour, but if you prefer this, I can switch to > utf-8 instead. I believe it's not really important which encoding to use (ascii or utf8) as it just needs calculate a hash of the string. So I believe you can merge the functions. https://codereview.chromium.org/2800133003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/inspector/DOMPatchSupport.cpp (right): https://codereview.chromium.org/2800133003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/DOMPatchSupport.cpp:434: string.length()); The length still doesn't match the data though. Could you please fix it? https://codereview.chromium.org/2800133003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/inspector/InspectorAnimationAgent.cpp (right): https://codereview.chromium.org/2800133003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/InspectorAnimationAgent.cpp:461: string.length()); ditto.
The CQ bit was checked by bratell@opera.com 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...
Realized I had reverted away the BUILD.gn change on my way home yesterday. :-) Restored the shared function and made it use utf-8 now. There can be some changes because the ascii function replaced unknown characters with ?, and I guess there won't be any unknown characters anymore.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Great! Thank you. lgtm https://codereview.chromium.org/2800133003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/inspector/AddStringToDigestor.cpp (right): https://codereview.chromium.org/2800133003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/inspector/AddStringToDigestor.cpp:17: } // namespace blink Also an empty string here please. https://codereview.chromium.org/2800133003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/inspector/AddStringToDigestor.h (right): https://codereview.chromium.org/2800133003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/inspector/AddStringToDigestor.h:16: #endif // AddStringToDigestor_h An empty string above this line plz.
The CQ bit was checked by bratell@opera.com
The patchset sent to the CQ was uploaded after l-g-t-m from alph@chromium.org Link to the patchset: https://codereview.chromium.org/2800133003/#ps120001 (title: "Adding spacer lines")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks, spaced the code like suggested! https://codereview.chromium.org/2800133003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/inspector/AddStringToDigestor.cpp (right): https://codereview.chromium.org/2800133003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/inspector/AddStringToDigestor.cpp:17: } // namespace blink On 2017/07/11 21:28:18, alph wrote: > Also an empty string here please. Done. https://codereview.chromium.org/2800133003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/inspector/AddStringToDigestor.h (right): https://codereview.chromium.org/2800133003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/inspector/AddStringToDigestor.h:16: #endif // AddStringToDigestor_h On 2017/07/11 21:28:18, alph wrote: > An empty string above this line plz. Done.
CQ is committing da patch.
Bot data: {"patchset_id": 120001, "attempt_start_ts": 1499860518698990,
"parent_rev": "3abacb6860a7a604643b302094ca1b30a96d4314", "commit_rev":
"b9c6b66c0e90591caa3154f176a135184fefcf7c"}
CQ is committing da patch.
Bot data: {"patchset_id": 120001, "attempt_start_ts": 1499860518698990,
"parent_rev": "2610a45c788375858c1796ae060f12acedd90ce4", "commit_rev":
"0ba26ac61aba0f76c0c3d948126264af5b43cd8b"}
Message was sent while issue was closed.
Description was changed from ========== Avoid duplicate functions: one AddStringToDigestor is enough There are two implementation of AddStringToDigestor. Beyond being one more than needed, they also conflict in jumbo builds. This patch merges the two AddStringToDigestor and removes the conflicting files from the jumbo exclusion list. ========== to ========== Avoid duplicate functions: one AddStringToDigestor is enough There are two implementation of AddStringToDigestor. Beyond being one more than needed, they also conflict in jumbo builds. This patch merges the two AddStringToDigestor and removes the conflicting files from the jumbo exclusion list. Review-Url: https://codereview.chromium.org/2800133003 Cr-Commit-Position: refs/heads/master@{#485992} Committed: https://chromium.googlesource.com/chromium/src/+/0ba26ac61aba0f76c0c3d9481262... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/chromium/src/+/0ba26ac61aba0f76c0c3d9481262... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
