Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PageSpeed console blank #1214

Closed
eldk opened this issue Dec 15, 2015 · 12 comments
Closed

PageSpeed console blank #1214

eldk opened this issue Dec 15, 2015 · 12 comments

Comments

@eldk
Copy link
Contributor

@eldk eldk commented Dec 15, 2015

Hello,

With mod_pagespeed 1.10.33.0-7562, url http://domain.tld/pagespeed_admin/console gives a 200 response but nothing is shown.

Other urls from /pagespeed_admin are goods.

Greatings,

Eric

@oschaaf
Copy link
Member

@oschaaf oschaaf commented Dec 18, 2015

I can reproduce it. Digging a little bit, manually calling pagespeed.startConsole() displays the page correctly. Was that call somehow lost?

@centminmod
Copy link

@centminmod centminmod commented Dec 22, 2015

i can confirm this happens on ngx_pagespeed 1.10 too

@jeffkaufman
Copy link
Contributor

@jeffkaufman jeffkaufman commented Mar 7, 2016

You can also see this not working on http://www.jefftk.com/pagespeed_admin/console

Looking at www.jefftk.com/pagespeed_admin/console?PageSpeedFilters=+debug I don't see google.setOnLoadCallback(pagespeed.startConsole); from pagespeed/system/console_start.js even though net/instaweb/instaweb.gyp contains 'js_includes': [ '<(DEPTH)/pagespeed/system/console_start.js' ], for both console_js_dbg and console_js_opt.

@jlporter : any ideas about what might have made the contents of console_start.js get dropped?

@jeffkaufman
Copy link
Contributor

@jeffkaufman jeffkaufman commented Mar 7, 2016

(I've now verified that this is still broken on trunk; we haven't incidentally fixed it.)

@jlporter
Copy link
Contributor

@jlporter jlporter commented Mar 7, 2016

Looks like this was probably broken when I open sourced the closure compiler flow. It should be fixed now by commit a0015d3.

@jeffkaufman jeffkaufman closed this Mar 8, 2016
@jeffkaufman jeffkaufman changed the title No more http://domain.tld/pagespeed_admin/console PageSpeed console blank Mar 8, 2016
@jmarantz
Copy link
Contributor

@jmarantz jmarantz commented Mar 8, 2016

Do we have a unit-test or system-test now that might catch this? E.g. that
a js symbol exists in the static js file that we serve?

On Tue, Mar 8, 2016 at 1:40 PM, Jeff Kaufman notifications@github.com
wrote:

Closed #1214 #1214.


Reply to this email directly or view it on GitHub
#1214 (comment).

@jeffkaufman
Copy link
Contributor

@jeffkaufman jeffkaufman commented Mar 8, 2016

With this change, we're now inlining the call to start it. You could write a system test for this, but a much more likely regression would be "we're still calling it, but now there's some sort of js error that keeps the console from being displayed". The right sort of test would be an automated browser test that makes sure there are no js errors and ideally that the console looks right. But that's a pain (and we don't currently have infrastructure set up for it.)

@jmarantz
Copy link
Contributor

@jmarantz jmarantz commented Mar 8, 2016

We're the call to a JavaScript function that we expect to be in a compiled
external file. I compiler-configuration problem that dead-code-eliminates
the entire file would cause a simple scan of the file for the expected
symbol to fail. This seems fairly simple.

On Tue, Mar 8, 2016 at 2:09 PM, Jeff Kaufman notifications@github.com
wrote:

With this change, we're now inlining the call to start it. You could write
a system test for this, but a much more likely regression would be "we're
still calling it, but now there's some sort of js error that keeps the
console from being displayed". The right sort of test would be an automated
browser test that makes sure there are no js errors and ideally that the
console looks right. But that's a pain (and we don't currently have
infrastructure set up for it.)


Reply to this email directly or view it on GitHub
#1214 (comment)
.

@jeffkaufman
Copy link
Contributor

@jeffkaufman jeffkaufman commented Mar 8, 2016

I'm still not sure what breakage you're trying to catch. Are you worried that a future compiler configuration problem would mean that pagespeed.startConsole would no longer be defined in our static js? Or something else?

(The problem with this bug was that pagespeed.startConsole was being defined but because of a mistake in how we set up our gyp files it wasn't being called.)

@crowell
Copy link
Contributor

@crowell crowell commented Mar 8, 2016

there is a test using phantom somewhere. that tests the admin site. It's
not currently part of the checkin suite, but perhaps it should be. (then we
have the downside of requiring phantomjs and casperjs)

On Tue, Mar 8, 2016 at 2:16 PM, Joshua Marantz notifications@github.com
wrote:

We're the call to a JavaScript function that we expect to be in a compiled
external file. I compiler-configuration problem that dead-code-eliminates
the entire file would cause a simple scan of the file for the expected
symbol to fail. This seems fairly simple.

On Tue, Mar 8, 2016 at 2:09 PM, Jeff Kaufman notifications@github.com
wrote:

With this change, we're now inlining the call to start it. You could
write
a system test for this, but a much more likely regression would be "we're
still calling it, but now there's some sort of js error that keeps the
console from being displayed". The right sort of test would be an
automated
browser test that makes sure there are no js errors and ideally that the
console looks right. But that's a pain (and we don't currently have
infrastructure set up for it.)


Reply to this email directly or view it on GitHub
<
#1214 (comment)

.


Reply to this email directly or view it on GitHub
#1214 (comment)
.

Jeff Crowell
https://github.com/crowell

@jeffkaufman
Copy link
Contributor

@jeffkaufman jeffkaufman commented Mar 8, 2016

(A test that checked whether pagespeed.startConsole was being defined would have passed, even when this was broken.)

@jeffkaufman
Copy link
Contributor

@jeffkaufman jeffkaufman commented Mar 31, 2016

Released in 1.11.33.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
7 participants