Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2813203002:
Introduce CertNodeBuilder in CertificateViewerDialogHandler::RequestCertificateFields (Closed)
|
Created:
3 years, 8 months ago by vabr (Chromium) Modified:
3 years, 8 months ago CC:
chromium-reviews Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionIntroduce CertNodeBuilder in CertificateViewerDialogHandler::RequestCertificateFields
CertificateViewerDialogHandler::RequestCertificateFields builds a DictionaryValue
representing certificate metadata. It contains a lot of boilerplate and
serializes what is a nested structure, thus making the code hard to understand.
This CL introduces a helper class for building that representation:
CertNodeBuilder. This class hides the boilerplate and through natural C++ code
indenting emphasises the data structure.
The change was originally motivated by ensuring that migrating
CertificateViewerDialogHandler::RequestCertificateFields from raw-pointer base::Value
API is correct. In the meantime, r463684 took care of that. But the additional
readability of the code might still be of value.
The CL also additionally changes raw owning base::Value pointers into unique_ptr
in other parts of the file.
BUG=581865
Review-Url: https://codereview.chromium.org/2813203002
Cr-Commit-Position: refs/heads/master@{#464805}
Committed: https://chromium.googlesource.com/chromium/src/+/2b9c3f652b91e4668bea6e821d177f4b639b0df0
Patch Set 1 #
Total comments: 9
Patch Set 2 : Get rid of DV::From #
Dependent Patchsets: Messages
Total messages: 26 (15 generated)
The CQ bit was checked by vabr@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 ========== Introduce CertNodeBuilder in CertificateViewerDialogHandler::ExportCertificate CertificateViewerDialogHandler::ExportCertificate builds a DictionaryValue representing certificate metadata. It contains a lot of boilerplate and serializes what is a nested structure, thus making the code hard to understand. This CL introduces a helper class for building that representation: CertNodeBuilder. This class hides the boilerplate and through natural C++ code indenting emphasises the data structure. The change was originally motivated by ensuring that migrating CertificateViewerDialogHandler::ExportCertificate from raw-pointer base::Value API is correct. In the meantime, r463684 took care of that. But the additional readability of the code might still be of value. BUG=581865 ========== to ========== Introduce CertNodeBuilder in CertificateViewerDialogHandler::ExportCertificate CertificateViewerDialogHandler::ExportCertificate builds a DictionaryValue representing certificate metadata. It contains a lot of boilerplate and serializes what is a nested structure, thus making the code hard to understand. This CL introduces a helper class for building that representation: CertNodeBuilder. This class hides the boilerplate and through natural C++ code indenting emphasises the data structure. The change was originally motivated by ensuring that migrating CertificateViewerDialogHandler::ExportCertificate from raw-pointer base::Value API is correct. In the meantime, r463684 took care of that. But the additional readability of the code might still be of value. The CL also additionally changes raw owning base::Value pointers into unique_ptr in other parts of the file. BUG=581865 ==========
vabr@chromium.org changed reviewers: + jdoerrie@chromium.org, michaelpg@chromium.org
Hi Jan and Michael! Could you please review this CL? Michael -- please do an OWNERS review. Jan -- please check whether handling of base::Value in the new code makes sense and does not take any steps backwards. Cheers, Vaclav
On 2017/04/12 12:20:07, vabr (Chromium) wrote: > Hi Jan and Michael! > > Could you please review this CL? > > Michael -- please do an OWNERS review. > > Jan -- please check whether handling of base::Value in the new code makes sense > and does not take any steps backwards. > > Cheers, > Vaclav Hi Vaclav, yes, this change LGTM! Thank you for doing this, it definitely will make switching to value semantics easier.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2813203002/diff/1/chrome/browser/ui/webui/cer... File chrome/browser/ui/webui/certificate_viewer_webui.cc (right): https://codereview.chromium.org/2813203002/diff/1/chrome/browser/ui/webui/cer... chrome/browser/ui/webui/certificate_viewer_webui.cc:110: base::MakeUnique<base::Value>(std::move(node_))); jdoerrie, correct me if I'm wrong, but could this be simplified? e.g.: return base::MakeUnique<base::DictionaryValue>(std::move(node_)); Otherwise, it: * move-constructs a Value from node_ * creates a temporary unique_ptr pointing to the new Value * calls From() with the temporary unique_ptr * verifies the unique_ptr points to a dictionary (it does) * copies unique_ptr's pointer and releases the unique_ptr * creates and returns a unique_ptr pointing to the copied pointer I think From() is intended for converting a unique_ptr of Value type to (possible) DictionaryValue type, but we know what |node_| is. https://codereview.chromium.org/2813203002/diff/1/chrome/browser/ui/webui/cer... chrome/browser/ui/webui/certificate_viewer_webui.cc:368: void CertificateViewerDialogHandler::RequestCertificateFields( In the CL title and description, did you mean to reference this function rather than ExportCertificate? https://codereview.chromium.org/2813203002/diff/1/chrome/browser/ui/webui/cer... chrome/browser/ui/webui/certificate_viewer_webui.cc:484: .Build()); If you like, we could remove all these .Build() boilerplate lines by adding CertNodeBuilder& CertNodeBuilder::Child(CertNodeBuilder&) that way, .Build() would only be needed when you need to use the DictionaryValue directly.
The CQ bit was checked by vabr@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...
Thanks to both of you for the review. Please have another look, Michael. Thanks! Vaclav https://codereview.chromium.org/2813203002/diff/1/chrome/browser/ui/webui/cer... File chrome/browser/ui/webui/certificate_viewer_webui.cc (right): https://codereview.chromium.org/2813203002/diff/1/chrome/browser/ui/webui/cer... chrome/browser/ui/webui/certificate_viewer_webui.cc:110: base::MakeUnique<base::Value>(std::move(node_))); On 2017/04/13 01:07:00, michaelpg wrote: > jdoerrie, correct me if I'm wrong, but could this be simplified? e.g.: > return base::MakeUnique<base::DictionaryValue>(std::move(node_)); > > Otherwise, it: > * move-constructs a Value from node_ > * creates a temporary unique_ptr pointing to the new Value > * calls From() with the temporary unique_ptr > * verifies the unique_ptr points to a dictionary (it does) > * copies unique_ptr's pointer and releases the unique_ptr > * creates and returns a unique_ptr pointing to the copied pointer > > I think From() is intended for converting a unique_ptr of Value type to > (possible) DictionaryValue type, but we know what |node_| is. Thanks for catching this! You are correct, I somehow missed that |node_| is already a DictionaryValue.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2813203002/diff/1/chrome/browser/ui/webui/cer... File chrome/browser/ui/webui/certificate_viewer_webui.cc (right): https://codereview.chromium.org/2813203002/diff/1/chrome/browser/ui/webui/cer... chrome/browser/ui/webui/certificate_viewer_webui.cc:110: base::MakeUnique<base::Value>(std::move(node_))); On 2017/04/13 06:20:26, vabr (Chromium) wrote: > On 2017/04/13 01:07:00, michaelpg wrote: > > jdoerrie, correct me if I'm wrong, but could this be simplified? e.g.: > > return base::MakeUnique<base::DictionaryValue>(std::move(node_)); > > > > Otherwise, it: > > * move-constructs a Value from node_ > > * creates a temporary unique_ptr pointing to the new Value > > * calls From() with the temporary unique_ptr > > * verifies the unique_ptr points to a dictionary (it does) > > * copies unique_ptr's pointer and releases the unique_ptr > > * creates and returns a unique_ptr pointing to the copied pointer > > > > I think From() is intended for converting a unique_ptr of Value type to > > (possible) DictionaryValue type, but we know what |node_| is. > > Thanks for catching this! You are correct, I somehow missed that |node_| is > already a DictionaryValue. Yes, you are right michaelpg@. Vaclav's updated changelist with dropping From is the way to go.
https://codereview.chromium.org/2813203002/diff/1/chrome/browser/ui/webui/cer... File chrome/browser/ui/webui/certificate_viewer_webui.cc (right): https://codereview.chromium.org/2813203002/diff/1/chrome/browser/ui/webui/cer... chrome/browser/ui/webui/certificate_viewer_webui.cc:368: void CertificateViewerDialogHandler::RequestCertificateFields( On 2017/04/13 01:07:00, michaelpg wrote: > In the CL title and description, did you mean to reference this function rather > than ExportCertificate? ping ^
Description was changed from ========== Introduce CertNodeBuilder in CertificateViewerDialogHandler::ExportCertificate CertificateViewerDialogHandler::ExportCertificate builds a DictionaryValue representing certificate metadata. It contains a lot of boilerplate and serializes what is a nested structure, thus making the code hard to understand. This CL introduces a helper class for building that representation: CertNodeBuilder. This class hides the boilerplate and through natural C++ code indenting emphasises the data structure. The change was originally motivated by ensuring that migrating CertificateViewerDialogHandler::ExportCertificate from raw-pointer base::Value API is correct. In the meantime, r463684 took care of that. But the additional readability of the code might still be of value. The CL also additionally changes raw owning base::Value pointers into unique_ptr in other parts of the file. BUG=581865 ========== to ========== Introduce CertNodeBuilder in CertificateViewerDialogHandler::RequestCertificateFields CertificateViewerDialogHandler::RequestCertificateFields builds a DictionaryValue representing certificate metadata. It contains a lot of boilerplate and serializes what is a nested structure, thus making the code hard to understand. This CL introduces a helper class for building that representation: CertNodeBuilder. This class hides the boilerplate and through natural C++ code indenting emphasises the data structure. The change was originally motivated by ensuring that migrating CertificateViewerDialogHandler::RequestCertificateFields from raw-pointer base::Value API is correct. In the meantime, r463684 took care of that. But the additional readability of the code might still be of value. The CL also additionally changes raw owning base::Value pointers into unique_ptr in other parts of the file. BUG=581865 ==========
Hi Michael! I'm sorry for missing the other 2 comments you posted, not sure how that happened. Please see my responses below; I fixed the description, but I hit an issue with removing the Build() calls. Cheers, Vaclav https://codereview.chromium.org/2813203002/diff/1/chrome/browser/ui/webui/cer... File chrome/browser/ui/webui/certificate_viewer_webui.cc (right): https://codereview.chromium.org/2813203002/diff/1/chrome/browser/ui/webui/cer... chrome/browser/ui/webui/certificate_viewer_webui.cc:368: void CertificateViewerDialogHandler::RequestCertificateFields( On 2017/04/13 01:07:00, michaelpg wrote: > In the CL title and description, did you mean to reference this function rather > than ExportCertificate? Indeed, thanks for catching that! https://codereview.chromium.org/2813203002/diff/1/chrome/browser/ui/webui/cer... chrome/browser/ui/webui/certificate_viewer_webui.cc:484: .Build()); On 2017/04/13 01:07:00, michaelpg wrote: > If you like, we could remove all these .Build() boilerplate lines by adding > CertNodeBuilder& CertNodeBuilder::Child(CertNodeBuilder&) > > that way, .Build() would only be needed when you need to use the DictionaryValue > directly. I like this, but the style guide forbids passing non-const references. Changing that to pointer arguments won't work either, because the compiler wisely advises against taking the address of a temporary. I could have Child accept CertNodeBuilder simply by value, but then the result of the passed expression would need to be an rvalue. That's the case for CertNodeBuilder(...) but not for CertNodeBuilder(...).Payload(...) etc., because Payload and Child return a reference, not a plain value. And they cannot return the plain value (something like return std::move(*this);) because that would break in case they are called on a named instance of a CertNodeBuilder (CNB builder; builder.Payload(...); would destroy the builder). I am not sure how to get rid of the intermediate Build() without adding more boilerplate elsewhere, or without violating the style guide. So I propose to keep the current code. What do you think?
lgtm https://codereview.chromium.org/2813203002/diff/1/chrome/browser/ui/webui/cer... File chrome/browser/ui/webui/certificate_viewer_webui.cc (right): https://codereview.chromium.org/2813203002/diff/1/chrome/browser/ui/webui/cer... chrome/browser/ui/webui/certificate_viewer_webui.cc:484: .Build()); On 2017/04/14 07:59:04, vabr (Chromium) wrote: > On 2017/04/13 01:07:00, michaelpg wrote: > > If you like, we could remove all these .Build() boilerplate lines by adding > > CertNodeBuilder& CertNodeBuilder::Child(CertNodeBuilder&) > > > > that way, .Build() would only be needed when you need to use the > DictionaryValue > > directly. > > I like this, but the style guide forbids passing non-const references. Changing > that to pointer arguments won't work either, because the compiler wisely advises > against taking the address of a temporary. > > I could have Child accept CertNodeBuilder simply by value, but then the result > of the passed expression would need to be an rvalue. That's the case for > CertNodeBuilder(...) but not for CertNodeBuilder(...).Payload(...) etc., because > Payload and Child return a reference, not a plain value. And they cannot return > the plain value (something like return std::move(*this);) because that would > break in case they are called on a named instance of a CertNodeBuilder (CNB > builder; builder.Payload(...); would destroy the builder). > > I am not sure how to get rid of the intermediate Build() without adding more > boilerplate elsewhere, or without violating the style guide. So I propose to > keep the current code. What do you think? yeah, any way I can think of would violate some style guide rule or another. I guess that's by design.
On 2017/04/14 18:08:32, michaelpg wrote: > lgtm > > https://codereview.chromium.org/2813203002/diff/1/chrome/browser/ui/webui/cer... > File chrome/browser/ui/webui/certificate_viewer_webui.cc (right): > > https://codereview.chromium.org/2813203002/diff/1/chrome/browser/ui/webui/cer... > chrome/browser/ui/webui/certificate_viewer_webui.cc:484: .Build()); > On 2017/04/14 07:59:04, vabr (Chromium) wrote: > > On 2017/04/13 01:07:00, michaelpg wrote: > > > If you like, we could remove all these .Build() boilerplate lines by adding > > > CertNodeBuilder& CertNodeBuilder::Child(CertNodeBuilder&) > > > > > > that way, .Build() would only be needed when you need to use the > > DictionaryValue > > > directly. > > > > I like this, but the style guide forbids passing non-const references. > Changing > > that to pointer arguments won't work either, because the compiler wisely > advises > > against taking the address of a temporary. > > > > I could have Child accept CertNodeBuilder simply by value, but then the result > > of the passed expression would need to be an rvalue. That's the case for > > CertNodeBuilder(...) but not for CertNodeBuilder(...).Payload(...) etc., > because > > Payload and Child return a reference, not a plain value. And they cannot > return > > the plain value (something like return std::move(*this);) because that would > > break in case they are called on a named instance of a CertNodeBuilder (CNB > > builder; builder.Payload(...); would destroy the builder). > > > > I am not sure how to get rid of the intermediate Build() without adding more > > boilerplate elsewhere, or without violating the style guide. So I propose to > > keep the current code. What do you think? > > yeah, any way I can think of would violate some style guide rule or another. I > guess that's by design. Thanks! I will land this after the branch point, so that there is enough time for fixing just in case I introduce any issues. Cheers, Vaclav
The CQ bit was checked by vabr@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jdoerrie@chromium.org Link to the patchset: https://codereview.chromium.org/2813203002/#ps20001 (title: "Get rid of DV::From")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 20001, "attempt_start_ts": 1492204693195300,
"parent_rev": "75104986e685df7cf9f44e4d4fcca97d19e9907c", "commit_rev":
"2b9c3f652b91e4668bea6e821d177f4b639b0df0"}
Message was sent while issue was closed.
Description was changed from ========== Introduce CertNodeBuilder in CertificateViewerDialogHandler::RequestCertificateFields CertificateViewerDialogHandler::RequestCertificateFields builds a DictionaryValue representing certificate metadata. It contains a lot of boilerplate and serializes what is a nested structure, thus making the code hard to understand. This CL introduces a helper class for building that representation: CertNodeBuilder. This class hides the boilerplate and through natural C++ code indenting emphasises the data structure. The change was originally motivated by ensuring that migrating CertificateViewerDialogHandler::RequestCertificateFields from raw-pointer base::Value API is correct. In the meantime, r463684 took care of that. But the additional readability of the code might still be of value. The CL also additionally changes raw owning base::Value pointers into unique_ptr in other parts of the file. BUG=581865 ========== to ========== Introduce CertNodeBuilder in CertificateViewerDialogHandler::RequestCertificateFields CertificateViewerDialogHandler::RequestCertificateFields builds a DictionaryValue representing certificate metadata. It contains a lot of boilerplate and serializes what is a nested structure, thus making the code hard to understand. This CL introduces a helper class for building that representation: CertNodeBuilder. This class hides the boilerplate and through natural C++ code indenting emphasises the data structure. The change was originally motivated by ensuring that migrating CertificateViewerDialogHandler::RequestCertificateFields from raw-pointer base::Value API is correct. In the meantime, r463684 took care of that. But the additional readability of the code might still be of value. The CL also additionally changes raw owning base::Value pointers into unique_ptr in other parts of the file. BUG=581865 Review-Url: https://codereview.chromium.org/2813203002 Cr-Commit-Position: refs/heads/master@{#464805} Committed: https://chromium.googlesource.com/chromium/src/+/2b9c3f652b91e4668bea6e821d17... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/2b9c3f652b91e4668bea6e821d17... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
