Closed Bug 1378865 Opened 7 years ago Closed 7 years ago

Stop using sdk/core/heritage in DevTools shared webconsole server logger

Categories

(DevTools :: General, enhancement, P1)

Product:
DevTools
Component:
General
Type:
enhancement

Tracking

(firefox56 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 56
Tracking Flags:
Tracking Status
firefox56 --- fixed

People

(Reporter: sole, Assigned: Honza)

References

Details

(Whiteboard: [nosdk])

Attachments

(1 file)

Bug 1378865 - Stop using sdk/core/heritage in DevTools shared webconsole server logger;
59 bytes, text/x-review-board-request
zer0
: review+
Details 
Used in: devtools/shared/webconsole/server-logger.js

More details to follow as we triage.
The attached patch:
- introduces `extend` in devtoolsutils.js (or is there a better place?)
- uses `extend` in webconsole

Honza
Assignee: nobody → odvarko
Flags: qe-verify-
Priority: -- → P3
Ignore comment #2, it belongs to different bug.

Honza
Comment on attachment 8884262 [details]
Bug 1378865 - Stop using sdk/core/heritage in DevTools shared webconsole server logger;

https://reviewboard.mozilla.org/r/155208/#review160252

::: commit-message-20f32:1
(Diff revision 2)
> +Bug 1378865 - Stop using sdk/core/heritage in DevTools shared webconsole server loggero; r=zer0

just a typo, "loggero", but maybe we can fix it before land the patch. :)

::: devtools/shared/webconsole/server-logger.js:37
(Diff revision 2)
>   * headers are parsed to find server side logs.
>   *
>   * A listeners for "http-on-examine-response" is registered when
>   * the listener starts and removed when destroy is executed.
>   */
> -var ServerLoggingListener = Class({
> +function ServerLoggingListener() {

We should use ES classes; I've updated our docs with all the examples and reference needed.
Attachment #8884262 - Flags: review?(zer0) → review-
> just a typo, "loggero",
Fixed

> We should use ES classes; I've updated our docs with all the examples and reference needed.
Keep prototype (discussed on IRC)

Honza
Status: NEW → ASSIGNED
Priority: P3 → P1
Whiteboard: [nosdk]
Target Milestone: --- → Firefox 56
Comment on attachment 8884262 [details]
Bug 1378865 - Stop using sdk/core/heritage in DevTools shared webconsole server logger;

https://reviewboard.mozilla.org/r/155208/#review163486

::: devtools/shared/webconsole/server-logger.js:38
(Diff revision 3)
>   *
>   * A listeners for "http-on-examine-response" is registered when
>   * the listener starts and removed when destroy is executed.
>   */
> -var ServerLoggingListener = Class({
> +function ServerLoggingListener() {
> +}

You have to copy the `Class`'s `initialize` method here, in the constructor. See: https://github.com/devtools-html/snippets-for-removing-the-sdk#how-to-replace-heritages-class
Attachment #8884262 - Flags: review?(zer0) → review-
I added r- since the constructor wasn't set at all, please ensure that the initialization code is in the constructor and its properly called. Also, mark the opened issue on mozreview as fixed once you've addressed them, thanks!
Thanks for the review, all fixed, thanks.

Honza
Comment on attachment 8884262 [details]
Bug 1378865 - Stop using sdk/core/heritage in DevTools shared webconsole server logger;

https://reviewboard.mozilla.org/r/155208/#review163592

r+, with the comment below addressed.

::: devtools/shared/webconsole/server-logger.js:38
(Diff revision 4)
>   *
>   * A listeners for "http-on-examine-response" is registered when
>   * the listener starts and removed when destroy is executed.
>   */
> -var ServerLoggingListener = Class({
> +function ServerLoggingListener(win, owner) {
> +  this.initialize(win, owner);

`initialize` method shouldn just be the same of `class`'s `constructor`, so you can just copy the body of the method in the `function`'s constructor, as noted in https://github.com/devtools-html/snippets-for-removing-the-sdk#how-to-deal-with-decorators (that I believe is exactly this case, with decorators).
Attachment #8884262 - Flags: review?(zer0) → review+
Comment resolved, thanks for the review Matteo!

Honza
Pushed by jodvarko@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/cfb0810a9bf3
Stop using sdk/core/heritage in DevTools shared webconsole server logger; r=zer0
We're sorry - something has gone wrong while rewriting or rebasing your commits. The commits being pushed no longer match what was requested. Please file a bug.
(In reply to Mozilla Autoland from comment #16)
> We're sorry - something has gone wrong while rewriting or rebasing your
> commits. The commits being pushed no longer match what was requested. Please
> file a bug.

[18:04]	glob	Honza: thanks.  i've seen this happen when someone requests the same change to land twice.  i have a patch under review that should prevent the duplicates.  i'll check if that's the case here
[18:05]	Honza	glob: should I file a bug or just ignore it?
[18:06]	glob	Honza: you can ignore it; what was landed is identical to what failed to land the 2nd time

So, we should be fine here.

Honza
https://hg.mozilla.org/mozilla-central/rev/cfb0810a9bf3
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: