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

Fix #631: Implements Thimble Console Back End. #624

Merged
merged 25 commits into from Mar 27, 2017

Conversation

Projects
None yet
3 participants
@raygervais

raygervais commented Mar 5, 2017

This is the first step toward implementing the suggested Javascript console

@raygervais

This comment has been minimized.

Show comment
Hide comment
@raygervais

raygervais Mar 7, 2017

@Pomax @humphd , I've updated the code to reflect the changes discussed.

raygervais commented Mar 7, 2017

@Pomax @humphd , I've updated the code to reflect the changes discussed.

@raygervais raygervais referenced this pull request Mar 7, 2017

Closed

Expand JavaScript Console Handling #631

6 of 8 tasks complete
@raygervais

This comment has been minimized.

Show comment
Hide comment
@raygervais

raygervais Mar 7, 2017

I created a new issue reflecting the next stages of implementation, and I've fixed your requested changes @humphd.

raygervais commented Mar 7, 2017

I created a new issue reflecting the next stages of implementation, and I've fixed your requested changes @humphd.

@humphd

This is starting to take shape. In addition to the fixes I've mentioned here, we'll also need to expand this a bit before we go further. I'd like to get a proper set of wrappers for all the various console.* methods. We can use some of the example implementations I've sent you in the issue (e.g., firebug-lite) as a guide.

@raygervais raygervais changed the title from Implements Thimble Console Log Back End. to Fixes #631: Implements Thimble Console Back End. Mar 15, 2017

@raygervais raygervais changed the title from Fixes #631: Implements Thimble Console Back End. to Fix #631: Implements Thimble Console Back End. Mar 15, 2017

@humphd

This comment has been minimized.

Show comment
Hide comment
@humphd

humphd Mar 16, 2017

Member

@raygervais please let @Pomax and me know when this is ready for review again.

Member

humphd commented Mar 16, 2017

@raygervais please let @Pomax and me know when this is ready for review again.

@raygervais

This comment has been minimized.

Show comment
Hide comment
@raygervais

raygervais Mar 16, 2017

@humphd, @Pomax the code is ready for another look over. I fixed the syntax issue which caused Travis to fail, and also simplified where I could at @Pomax's request. Included below is how the console currently functions and how I tested said console functions.

If we are content with the code changes, I'd be welcome to closing both this and the issue #631 and creating a new issue related to the UI of the console.

screenshot_20170316_183411

raygervais commented Mar 16, 2017

@humphd, @Pomax the code is ready for another look over. I fixed the syntax issue which caused Travis to fail, and also simplified where I could at @Pomax's request. Included below is how the console currently functions and how I tested said console functions.

If we are content with the code changes, I'd be welcome to closing both this and the issue #631 and creating a new issue related to the UI of the console.

screenshot_20170316_183411

@raygervais

This comment has been minimized.

Show comment
Hide comment
@raygervais

raygervais Mar 20, 2017

@humphd, Thank you for the code review. I will get to these changes immediately. When I am done, would you like me to branch off while this one awaits code review and work on the Console UI? Or, commit to here?

PS, thank you for clarifying some of the syntactical issues I had previously, I do appreciate learning the most optimal way for Mozilla-esque development and concerns.

raygervais commented Mar 20, 2017

@humphd, Thank you for the code review. I will get to these changes immediately. When I am done, would you like me to branch off while this one awaits code review and work on the Console UI? Or, commit to here?

PS, thank you for clarifying some of the syntactical issues I had previously, I do appreciate learning the most optimal way for Mozilla-esque development and concerns.

@humphd

This comment has been minimized.

Show comment
Hide comment
@humphd

humphd Mar 20, 2017

Member

The thing with git is that you can fork off of this and start your UI, then just rebase later onto master when your backend stuff lands. You don't need to block.

Don't push any more new code to this branch, do all new stuff (except for fixes) on new PRs and branches.

Member

humphd commented Mar 20, 2017

The thing with git is that you can fork off of this and start your UI, then just rebase later onto master when your backend stuff lands. You don't need to block.

Don't push any more new code to this branch, do all new stuff (except for fixes) on new PRs and branches.

Fixed code to reflect requested changes, including:
- Removed repeated code in ConsoleManagerRemote.js
- Switched arguments in transportSend to be in logical order in ConsoleManagerRemote.js
- Updated Data to be an object which contains type and arguments in PostManagerTransportRemote.js
- Added 'TODO:' to ConsoleManager.js
- Removed 'Bramble Console' test argument from ConsoleManager.js
- Removed 'Runtime.Evaluate' check in ConsoleManagerRemote.js
- Added a default to Console.Log in the case that Console['Type'] is not specified
- Added time, and timeEnd functions to the forEach loop mentioend in first bullet point
- Removed time, timeEnd argument handling entirely, since the removal of 'Bramble Console' test argument no longer is first argument
@raygervais

This comment has been minimized.

Show comment
Hide comment
@raygervais

raygervais Mar 21, 2017

@humphd, I've updated the files with your requested changes. When you get a chance the code is ready for another review, and in the mean time I will keep working using another forked-branch on the user interface.

raygervais commented Mar 21, 2017

@humphd, I've updated the files with your requested changes. When you get a chance the code is ready for another review, and in the mean time I will keep working using another forked-branch on the user interface.

@humphd

Few small things, and I think this is ready! Let me know what you've fixed these.

@raygervais

This comment has been minimized.

Show comment
Hide comment
@raygervais

raygervais Mar 23, 2017

@humphd, I've fixed all your requested changes, and am ready for another review when you get the chance.

raygervais commented Mar 23, 2017

@humphd, I've fixed all your requested changes, and am ready for another review when you get the chance.

@humphd

Close, but not quite.

@humphd humphd merged commit e2b3985 into mozilla:master Mar 27, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@humphd

This comment has been minimized.

Show comment
Hide comment
@humphd

humphd Mar 27, 2017

Member

Thank you! Excited to see the UI happen next.

Member

humphd commented Mar 27, 2017

Thank you! Excited to see the UI happen next.

@raygervais

This comment has been minimized.

Show comment
Hide comment
@raygervais

raygervais Mar 27, 2017

Thank you @humphd, @Pomax for the code reviews and stylistic pointers!

raygervais commented Mar 27, 2017

Thank you @humphd, @Pomax for the code reviews and stylistic pointers!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment