1302378 - Add more v4 debug message

Status: RESOLVED WONTFIX

(Toolkit :: Safe Browsing, defect, P3)

Comment 1

[Henry Chang [:hchang]]

Attachment: Bug 1302378 - Add v4 debug message. .

Review commit: https://reviewboard.mozilla.org/r/78348/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/78348/

Comment 2

[François Marier [:francois]]

Comment on attachment 8790635 [details]
Bug 1302378 - Add v4 debug message. .

https://reviewboard.mozilla.org/r/78348/#review77094

::: toolkit/components/url-classifier/content/listmanager.js:410
(Diff revision 1)
>      });
>  
>      let urlUtils = Cc["@mozilla.org/url-classifier/utils;1"]
>                       .getService(Ci.nsIUrlClassifierUtils);
> +
> +    log("makeUpdateRequestV4 with " + JSON.stringify(tableArray) +

nit: to mirror the other debug message for V2, we could change the message to

    log("update request (V4): " + JSON.stringify(tableArray) + " and " + JSON.stringify(stateArray));

::: toolkit/components/url-classifier/content/listmanager.js:440
(Diff revision 1)
>      }
>  
>      streamerMap.isPostRequest = true;
>    }
>  
>    log("update request: " + JSON.stringify(streamerMap, undefined, 2) + "\n");

nit: we could rename this one to include the version number too

    log("update request (V2): " + JSON.stringify(streamerMap, undefined, 2) + "\n");

::: toolkit/components/url-classifier/nsUrlClassifierStreamUpdater.cpp:206
(Diff revision 1)
>    LOG(("(pre) Fetching update from %s\n", PromiseFlatCString(aUpdateUrl).get()));
>  
>    nsCString updateUrl(aUpdateUrl);
>    if (!aIsPostRequest) {
>      updateUrl.AppendPrintf("&$req=%s", nsCString(aRequestPayload).get());
> +    LOG(("Appended V4 UpdateURL: %s", updateUrl.get()));

nit: "Full V4 UpdateURL" is a bit clearer than "Appended V4 UpdateURL"

Comment 3

[François Marier [:francois]]

(In reply to François Marier [:francois] from comment #2)
> ::: toolkit/components/url-classifier/content/listmanager.js:410
> (Diff revision 1)
> >      });
> >  
> >      let urlUtils = Cc["@mozilla.org/url-classifier/utils;1"]
> >                       .getService(Ci.nsIUrlClassifierUtils);
> > +
> > +    log("makeUpdateRequestV4 with " + JSON.stringify(tableArray) +
> 
> nit: to mirror the other debug message for V2, we could change the message to
> 
>     log("update request (V4): " + JSON.stringify(tableArray) + " and " +
> JSON.stringify(stateArray));
> 
> ::: toolkit/components/url-classifier/content/listmanager.js:440
> (Diff revision 1)
> >      }
> >  
> >      streamerMap.isPostRequest = true;
> >    }
> >  
> >    log("update request: " + JSON.stringify(streamerMap, undefined, 2) + "\n");
> 
> nit: we could rename this one to include the version number too
> 
>     log("update request (V2): " + JSON.stringify(streamerMap, undefined, 2)
> + "\n");
> 

Ignore these two comments, I got confused with the indentation. The second message is not V2-specific, it shows up on both.

The original log() message you have is all good.

Comment 4

[Henry Chang [:hchang]]

Thanks for the review :)

To be honest, I don't know if these debug messages are helpful. The message I added in the patch are dedicated to the input and output of the request payload given that the error message from server said it's something like bad base64 encoded data.

Anyways, if you don't have any concern landing this patch I'll land it as soon as possible! Thanks!

Comment 5

[François Marier [:francois]]

(In reply to Henry Chang [:henry][:hchang] from comment #4)
> Anyways, if you don't have any concern landing this patch I'll land it as
> soon as possible! Thanks!

Ship it! :)

Comment 6

[Henry Chang [:hchang]]

I now feel like these debug message doesn't seem to be helpful after fixing some bugs. Just remove from the milestone but leave it open, sit down and figure out what kind of debug message would help.

Comment 7

[François Marier [:francois]]

(In reply to Henry Chang [:henry][:hchang] from comment #6)
> I now feel like these debug message doesn't seem to be helpful after fixing
> some bugs. Just remove from the milestone but leave it open, sit down and
> figure out what kind of debug message would help.

If you think these messages might be helpful, let's land the patch, otherwise, let's close the bug and mark it as wontfix. We can always create a new bug later if we think of new debug messages that might be helpful.

Comment 8

[Henry Chang [:hchang]]

I don't think the message I add in the patch very helpful (at least so far :p ). Mark WONTFIX.