Opened 6 years ago

Closed 6 years ago

Last modified 6 years ago

#6189 closed Bug (fixed)

Reducing the code size

Reported by: Saare Owned by: Saare
Priority: Normal Milestone: CKEditor 3.4.2
Component: General Version:
Keywords: Doc? Cc:

Description (last modified by Saare)

A part of a discussion with Fred:

Clearly the code is constantly getting bigger. So how about reviewing the code and try to reduce it: use 1 and 0 for true and false when possible, remove the "defined" configuration when possible (leaving just comments about them of course) etc.

Attachments (2)

6189.patch (107.1 KB) - added by Saare 6 years ago.
6189_2.patch (114.7 KB) - added by Saare 6 years ago.

Download all attachments as: .zip

Change History (16)

comment:1 Changed 6 years ago by Saare

  • Description modified (diff)
  • Status changed from new to assigned

comment:2 Changed 6 years ago by alfonsoml

I think that all the true/false -> 1/0 conversions should be done at package time. That way the code remains as is and it will be future-proof.

comment:3 Changed 6 years ago by fredck

@alfonsoml, I think we're using the strict equality operator in some places, which would break things. Maybe better to have the 1/0 thing is a coding style best practice, so we have better control of it.

Changed 6 years ago by Saare

comment:4 Changed 6 years ago by Saare

  • Status changed from assigned to review

These changes should become routine for the core developers as well as the contributers, so adding documentation regarding flags and initializing of config settings is recommended IMHO.

comment:5 follow-up: Changed 6 years ago by garry.yao

Isn't this suppose to be a [MicroChanges micro-change]?

comment:6 Changed 6 years ago by Saare

That was my initial thought, but Fred asked me to open a ticket for that and put the patch on review.

comment:7 follow-up: Changed 6 years ago by garry.yao

  • Status changed from review to review_failed

In which sense does the lazy config option initialization style contribute to performance? Anway some of the styles configs are also used by 'pastefromword' plugin thus get broken. Besides, some of the changes in the patch are CKPackager's task instead, e.g. quotes transformation, simple block braces, check this out.

comment:8 Changed 6 years ago by garry.yao

For me this's pretty much a packager ticket really, the 1st feature I would vote would be an invariant variable feature that caches reference like editor.lang.common.generalTab will contribute a lot to the reduction.

Changed 6 years ago by Saare

comment:9 in reply to: ↑ 7 ; follow-up: Changed 6 years ago by Saare

  • Status changed from review_failed to review

Replying to garry.yao:

Besides, some of the changes in the patch are CKPackager's task instead, e.g. quotes transformation, simple block braces, check this out.

These are just some coding style fixes I added.

the 1st feature I would vote would be an invariant variable feature that caches reference like editor.lang.common.generalTab will contribute a lot to the reduction.

I agree, that could be great, but let's do it ourselves in the meanwhile.

comment:10 in reply to: ↑ 9 Changed 6 years ago by garry.yao

the 1st feature I would vote would be an invariant variable feature that caches reference like editor.lang.common.generalTab will contribute a lot to the reduction.

I agree, that could be great, but let's do it ourselves in the meanwhile.

Moved to #6475, so don't worry about it now.

comment:11 in reply to: ↑ 5 Changed 6 years ago by garry.yao

  • Status changed from review to review_passed

1kb of footprint reduction is produced after the patch.

comment:12 Changed 6 years ago by Saare

  • Keywords Doc? added
  • Resolution set to fixed
  • Status changed from review_passed to closed

Fixed with [5949]. Additional minor fixes should be commited as micro changes.
These small things should become routine, let's have a documentation for them.

comment:13 Changed 6 years ago by Saare

  • Milestone changed from CKEditor 3.5 to CKEditor 3.4.2

comment:14 Changed 6 years ago by fredck

A note... it was wrong to have such big change in a micro release. That's why it has been targeted to the 3.5 earlier. More attention is required in the future.

Another note... just to remember: the "this" trick is not needed. CKPackager automatically replaces "this" if there are enough occurrences of it to compensate it. Anyway, there is no need to change what has already been changed.

Note: See TracTickets for help on using tickets.
© 2003 – 2016 CKSource – Frederico Knabben. All rights reserved. | Terms of use | Privacy policy