Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2044503002:
Refactor the naming scheme of Deserialize* functions in form_field_data.cc (Closed)
|
Created:
4 years, 6 months ago by Hwanseung Lee(hs1217.lee) Modified:
4 years, 6 months ago Reviewers:
vabr (Chromium) CC:
chromium-reviews, rouslan+autofill_chromium.org, estade+watch_chromium.org, vabr+watchlistautofill_chromium.org, browser-components-watch_chromium.org, jdonnelly+autofillwatch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRefactor the naming scheme of Deserialize* functions in form_field_data.cc
This is needed for https://codereview.chromium.org/2022123002/.
BUG=614280
Committed: https://crrev.com/fe5f05044f43fe5f1ef9806fc6b178992df8f615
Cr-Commit-Position: refs/heads/master@{#398409}
Patch Set 1 #Patch Set 2 : #Patch Set 3 : #
Total comments: 3
Patch Set 4 : #
Total comments: 3
Patch Set 5 : #Messages
Total messages: 23 (11 generated)
Description was changed from ========== Seperate Deserialize* function for refectoring. Seperate Deserialize* function for refectoring. is_checked & is_checkable will be merged to one enum value. so kPickleVersion will be update to 4. it is first step for refectoring. BUG=614280 ========== to ========== Seperate Deserialize* function for refectoring. is_checked & is_checkable will be merged to one enum value. so kPickleVersion will be update to 4. it is first step for refectoring. BUG=614280 ==========
hs1217.lee@gmail.com changed reviewers: + vabr@chromium.org
please review this commit.
Description was changed from ========== Seperate Deserialize* function for refectoring. is_checked & is_checkable will be merged to one enum value. so kPickleVersion will be update to 4. it is first step for refectoring. BUG=614280 ========== to ========== Separate Deserialize* function for refectoring. is_checked & is_checkable will be merged to one enum value. so kPickleVersion will be update to 4. it is first step for refactoring. BUG=614280 ==========
Description was changed from ========== Separate Deserialize* function for refectoring. is_checked & is_checkable will be merged to one enum value. so kPickleVersion will be update to 4. it is first step for refactoring. BUG=614280 ========== to ========== Separate Deserialize* function for refactoring. is_checked & is_checkable will be merged to one enum value. so kPickleVersion will be update to 4. it is first step for refactoring. BUG=614280 ==========
Thanks for the quick work! This looks good, but please have a look at my comments below. Also, let's improve the CL title and message a little bit: (1) The title does not mention the main purpose of the CL, which is renaming the methods. It also does not hint at where the change happens. My suggestion would be "Refactor the naming scheme of Deserialize* functions in form_field_data.cc" (2) Instead of mentioning the merging of is_checked and is_checkable, just drop there a link to https://codereview.chromium.org/2022123002/. You can say something like: Refactor the naming scheme of Deserialize* functions in form_field_data.cc This is needed for https://codereview.chromium.org/2022123002/. BUG=614280 https://codereview.chromium.org/2044503002/diff/40001/components/autofill/cor... File components/autofill/core/common/form_field_data.cc (right): https://codereview.chromium.org/2044503002/diff/40001/components/autofill/cor... components/autofill/core/common/form_field_data.cc:53: bool DeserializeCommonSection1(base::PickleIterator* iter, Sorry for nitpicking, but I would still suggest to name the functions in an easy sequential scheme, like: DeserializeSection1, DeserializeSection2, DeserializeSection3, etc. The long names are hard to read, the parts like "Common" bring little useful information (the fact that they are common is apparent from them appearing everywhere). Using numbers both to refer to sequence and the versions is also confusing. https://codereview.chromium.org/2044503002/diff/40001/components/autofill/cor... components/autofill/core/common/form_field_data.cc:64: bool DeserializeCheckStaus(base::PickleIterator* iter, typo: Staus -> Status Having said that, this name would become confusing in your new patch (as this part will actually be replaced by check_status). Please just do the DeserializeSection<number> as advised above. https://codereview.chromium.org/2044503002/diff/40001/components/autofill/cor... components/autofill/core/common/form_field_data.cc:64: bool DeserializeCheckStaus(base::PickleIterator* iter, Actually, let's not split DeserializeCommonSection1 here at all, keep this CL just for the renaming. The split belongs to your main CL (https://codereview.chromium.org/2022123002/). When you do it there, you will notice that the second and third part split from what you'll rename to DeserializeSection1 will need to take numbers 5 and 6, because 2 and 3 will be already taken. But that's no problem. The main point will be that you won't have to edit the deserialization code for older versions at all, and the simple names like "DeserializeSection2" will not need to be changed (as they are not tied to what is actually done and for which version and where).
Description was changed from ========== Separate Deserialize* function for refactoring. is_checked & is_checkable will be merged to one enum value. so kPickleVersion will be update to 4. it is first step for refactoring. BUG=614280 ========== to ========== Refactor the naming scheme of Deserialize* functions in form_field_data.cc is_checked & is_checkable will be merged to one enum value. so kPickleVersion will be update to 4. it is first step for refactoring. BUG=614280 ==========
Description was changed from ========== Refactor the naming scheme of Deserialize* functions in form_field_data.cc is_checked & is_checkable will be merged to one enum value. so kPickleVersion will be update to 4. it is first step for refactoring. BUG=614280 ========== to ========== Refactor the naming scheme of Deserialize* functions in form_field_data.cc This is needed for https://codereview.chromium.org/2022123002/. BUG=614280 ==========
On 2016/06/06 15:40:02, vabr (Chromium) wrote: > Thanks for the quick work! > This looks good, but please have a look at my comments below. > > Also, let's improve the CL title and message a little bit: > > (1) The title does not mention the main purpose of the CL, which is renaming the > methods. It also does not hint at where the change happens. My suggestion would > be "Refactor the naming scheme of Deserialize* functions in form_field_data.cc" > > (2) Instead of mentioning the merging of is_checked and is_checkable, just drop > there a link to https://codereview.chromium.org/2022123002/. You can say > something like: > > Refactor the naming scheme of Deserialize* functions in form_field_data.cc > > This is needed for https://codereview.chromium.org/2022123002/. > > BUG=614280 > > https://codereview.chromium.org/2044503002/diff/40001/components/autofill/cor... > File components/autofill/core/common/form_field_data.cc (right): > > https://codereview.chromium.org/2044503002/diff/40001/components/autofill/cor... > components/autofill/core/common/form_field_data.cc:53: bool > DeserializeCommonSection1(base::PickleIterator* iter, > Sorry for nitpicking, but I would still suggest to name the functions in an easy > sequential scheme, like: > DeserializeSection1, DeserializeSection2, DeserializeSection3, etc. > > The long names are hard to read, the parts like "Common" bring little useful > information (the fact that they are common is apparent from them appearing > everywhere). Using numbers both to refer to sequence and the versions is also > confusing. > > https://codereview.chromium.org/2044503002/diff/40001/components/autofill/cor... > components/autofill/core/common/form_field_data.cc:64: bool > DeserializeCheckStaus(base::PickleIterator* iter, > typo: Staus -> Status > > Having said that, this name would become confusing in your new patch (as this > part will actually be replaced by check_status). Please just do the > DeserializeSection<number> as advised above. > > https://codereview.chromium.org/2044503002/diff/40001/components/autofill/cor... > components/autofill/core/common/form_field_data.cc:64: bool > DeserializeCheckStaus(base::PickleIterator* iter, > Actually, let's not split DeserializeCommonSection1 here at all, keep this CL > just for the renaming. The split belongs to your main CL > (https://codereview.chromium.org/2022123002/). > > When you do it there, you will notice that the second and third part split from > what you'll rename to DeserializeSection1 will need to take numbers 5 and 6, > because 2 and 3 will be already taken. But that's no problem. The main point > will be that you won't have to edit the deserialization code for older versions > at all, and the simple names like "DeserializeSection2" will not need to be > changed (as they are not tied to what is actually done and for which version and > where). could you review again? i just rename function by your guide.
On 2016/06/06 15:40:02, vabr (Chromium) wrote: > Thanks for the quick work! > This looks good, but please have a look at my comments below. > > Also, let's improve the CL title and message a little bit: > > (1) The title does not mention the main purpose of the CL, which is renaming the > methods. It also does not hint at where the change happens. My suggestion would > be "Refactor the naming scheme of Deserialize* functions in form_field_data.cc" > > (2) Instead of mentioning the merging of is_checked and is_checkable, just drop > there a link to https://codereview.chromium.org/2022123002/. You can say > something like: > > Refactor the naming scheme of Deserialize* functions in form_field_data.cc > > This is needed for https://codereview.chromium.org/2022123002/. > > BUG=614280 > > https://codereview.chromium.org/2044503002/diff/40001/components/autofill/cor... > File components/autofill/core/common/form_field_data.cc (right): > > https://codereview.chromium.org/2044503002/diff/40001/components/autofill/cor... > components/autofill/core/common/form_field_data.cc:53: bool > DeserializeCommonSection1(base::PickleIterator* iter, > Sorry for nitpicking, but I would still suggest to name the functions in an easy > sequential scheme, like: > DeserializeSection1, DeserializeSection2, DeserializeSection3, etc. > > The long names are hard to read, the parts like "Common" bring little useful > information (the fact that they are common is apparent from them appearing > everywhere). Using numbers both to refer to sequence and the versions is also > confusing. > > https://codereview.chromium.org/2044503002/diff/40001/components/autofill/cor... > components/autofill/core/common/form_field_data.cc:64: bool > DeserializeCheckStaus(base::PickleIterator* iter, > typo: Staus -> Status > > Having said that, this name would become confusing in your new patch (as this > part will actually be replaced by check_status). Please just do the > DeserializeSection<number> as advised above. > > https://codereview.chromium.org/2044503002/diff/40001/components/autofill/cor... > components/autofill/core/common/form_field_data.cc:64: bool > DeserializeCheckStaus(base::PickleIterator* iter, > Actually, let's not split DeserializeCommonSection1 here at all, keep this CL > just for the renaming. The split belongs to your main CL > (https://codereview.chromium.org/2022123002/). > > When you do it there, you will notice that the second and third part split from > what you'll rename to DeserializeSection1 will need to take numbers 5 and 6, > because 2 and 3 will be already taken. But that's no problem. The main point > will be that you won't have to edit the deserialization code for older versions > at all, and the simple names like "DeserializeSection2" will not need to be > changed (as they are not tied to what is actually done and for which version and > where). could you review again? i just rename function by your guide.
Thank you! This is looking better. Just a couple of minor comments, almost there. Cheers, Vaclav https://codereview.chromium.org/2044503002/diff/60001/components/autofill/cor... File components/autofill/core/common/form_field_data.cc (right): https://codereview.chromium.org/2044503002/diff/60001/components/autofill/cor... components/autofill/core/common/form_field_data.cc:68: bool DeserializeVersion2Specific(base::PickleIterator* iter, nit: This should also have the name of the form DeserializeSection<number>. https://codereview.chromium.org/2044503002/diff/60001/components/autofill/cor... components/autofill/core/common/form_field_data.cc:73: bool DeserializeSection2(base::PickleIterator* iter, Please do not move this, just rename. That way git blame will only point to your CL at the first line, making digging through history easier. https://codereview.chromium.org/2044503002/diff/60001/components/autofill/cor... components/autofill/core/common/form_field_data.cc:80: bool DeserializeVersion3Specific(base::PickleIterator* iter, nit: This should also have the name of the form DeserializeSection<number>.
On 2016/06/07 07:14:22, vabr (Chromium) wrote: > Thank you! This is looking better. Just a couple of minor comments, almost > there. > > Cheers, > Vaclav > > https://codereview.chromium.org/2044503002/diff/60001/components/autofill/cor... > File components/autofill/core/common/form_field_data.cc (right): > > https://codereview.chromium.org/2044503002/diff/60001/components/autofill/cor... > components/autofill/core/common/form_field_data.cc:68: bool > DeserializeVersion2Specific(base::PickleIterator* iter, > nit: This should also have the name of the form DeserializeSection<number>. > > https://codereview.chromium.org/2044503002/diff/60001/components/autofill/cor... > components/autofill/core/common/form_field_data.cc:73: bool > DeserializeSection2(base::PickleIterator* iter, > Please do not move this, just rename. That way git blame will only point to your > CL at the first line, making digging through history easier. > > https://codereview.chromium.org/2044503002/diff/60001/components/autofill/cor... > components/autofill/core/common/form_field_data.cc:80: bool > DeserializeVersion3Specific(base::PickleIterator* iter, > nit: This should also have the name of the form DeserializeSection<number>. could you review this new patchset?
Thanks, this LGTM! Cheers, Vaclav
The CQ bit was checked by hs1217.lee@gmail.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2044503002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by hs1217.lee@gmail.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2044503002/80001
Message was sent while issue was closed.
Description was changed from ========== Refactor the naming scheme of Deserialize* functions in form_field_data.cc This is needed for https://codereview.chromium.org/2022123002/. BUG=614280 ========== to ========== Refactor the naming scheme of Deserialize* functions in form_field_data.cc This is needed for https://codereview.chromium.org/2022123002/. BUG=614280 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Refactor the naming scheme of Deserialize* functions in form_field_data.cc This is needed for https://codereview.chromium.org/2022123002/. BUG=614280 ========== to ========== Refactor the naming scheme of Deserialize* functions in form_field_data.cc This is needed for https://codereview.chromium.org/2022123002/. BUG=614280 Committed: https://crrev.com/fe5f05044f43fe5f1ef9806fc6b178992df8f615 Cr-Commit-Position: refs/heads/master@{#398409} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/fe5f05044f43fe5f1ef9806fc6b178992df8f615 Cr-Commit-Position: refs/heads/master@{#398409} |
